Skip to content
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.

Sign up for GitHub

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

Jump to bottom

clang-format: Use config directly instead of modifying repository [AP-3053] #146

Merged
merged 2 commits into from
Dec 10, 2024
Merged

clang-format: Use config directly instead of modifying repository [AP-3053] #146

merged 2 commits into from
Dec 10, 2024

Conversation

sbmueller
Copy link
Contributor

@sbmueller sbmueller commented Dec 10, 2024 •
edited
Loading

The clang-format logic relied on copying the config file into the repo root, which pollutes the repository and might fail if there is already a symlink. This change modifies the logic to directly pass the config location to the clang-format CLI.

Tested in starling-core:

bazel run @rules_swiftnav//clang_format
INFO: Analyzed target @rules_swiftnav//clang_format:clang_format (0 packages loaded, 0 targets configured).
INFO: Found 1 target...
Target @rules_swiftnav//clang_format:clang_format up-to-date:
  bazel-bin/external/rules_swiftnav/clang_format/clang_format
INFO: Elapsed time: 0.255s, Critical Path: 0.12s
INFO: 1 process: 1 internal.
INFO: Build completed successfully, 1 total action
INFO: Running command line: bazel-bin/external/rules_swiftnav/clang_format/clang_format format_diff external/aarch64-darwin-llvm/bin/clang-format external/rules_swiftnav/clang_format/.clang-format

@sbmueller sbmueller requested a review from armallen December 10, 2024 14:32
@sbmueller sbmueller requested a review from a team as a code owner December 10, 2024 14:32
@sbmueller sbmueller changed the title clang-format: Use config directly instead of modifying repository clang-format: Use config directly instead of modifying repository [AP-3053] Dec 10, 2024
armallen
armallen approved these changes Dec 10, 2024
View reviewed changes
Copy link

@armallen armallen left a comment

Choose a reason for hiding this comment

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

Thanks for looking into this 🚀

clang_format/run_clang_format.sh Outdated
git ls-files '*.[ch]' '*.cpp' '*.cxx' '*.cc' '*.hpp' '*.hxx' \
| xargs -r $CLANG_FORMAT_BIN -i
| xargs -r $CLANG_FORMAT_BIN -i --style=file:$CLANG_FORMAT_CONFIG
Copy link

Choose a reason for hiding this comment

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

nit: ShellCheck reports several issues in this script (missing double quotes around all variables, uses of backquotes), which might be worth fixing?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Cool linter! I've fixed all the smells.

@sbmueller sbmueller merged commit e1db23f into main Dec 10, 2024
1 check passed
@sbmueller sbmueller deleted the smuller/AP-3053-clang-tools branch December 10, 2024 16:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@armallen armallen armallen approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@sbmueller @armallen