Skip to content

CXX-3198 support clang-format-all.sh on macOS #1307

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 3 commits into from
Dec 30, 2024

Conversation

kevinAlbs
Copy link
Collaborator

Follow-up to #1299. Verified with this patch build.

Use grep instead of -regextype to fix observed error running etc/clang-format-all.sh on macOS:

find: -regextype: unknown primary or operator

Also allow overriding the used clang-format. This is intended to ease my current set-up and can be reverted if desired. I installed LLVM 1.19 with brew install llvm, and it is not on a system path to avoid interfering with AppleClang.

@kevinAlbs kevinAlbs requested a review from eramongodb December 30, 2024 20:09
Copy link
Contributor

@eramongodb eramongodb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Minor suggestions; otherwise, LGTM.

Comment on lines 15 to 16
CLANG_FORMAT="${CLANG_FORMAT:-clang-format}"
"$CLANG_FORMAT" --version
Copy link
Contributor

@eramongodb eramongodb Dec 30, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
CLANG_FORMAT="${CLANG_FORMAT:-clang-format}"
"$CLANG_FORMAT" --version
clang_format_binary="${CLANG_FORMAT_BINARY:-clang-format}"
"${clang_format_binary:?}" --version

For consistency with DOXYGEN_BINARY, POETRY_PYTHON_BINARY (C Driver), etc. + recommend using separate variables for input env var (UPPER_CASE) and local variable (snake_case) + use ${clang_format_binary:?} to ensure not-null, not-empty when used below (e.g. catch typoes, etc.).

@@ -21,11 +22,11 @@ source_dirs=(
)

mapfile -t source_files < <(
find "${source_dirs[@]:?}" -regextype posix-egrep -regex '.*\.(hpp|hh|cpp)'
find "${source_dirs[@]:?}" | grep -E '.*\.(hpp|hh|cpp)$'
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Although I would prefer to use PCRE (-P) for consistency, IIRC it is not fully supported across all distros/versions, so I am fine with using -E here for now.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unfortunately -P does not appear supported on macOS grep:

% grep -P
grep: invalid option -- P
usage: grep [-abcdDEFGHhIiJLlMmnOopqRSsUVvwXxZz] [-A num] [-B num] [-C[num]]
        [-e pattern] [-f file] [--binary-files=value] [--color=when]
        [--context[=num]] [--directories=action] [--label] [--line-buffered]
        [--null] [pattern] [file ...]

@kevinAlbs kevinAlbs merged commit c81d281 into mongodb:master Dec 30, 2024
2 of 3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants