Skip to content

[nfc]For InstrProfData.inc, clang-format functions and opt-out of formatting on the rest #82057

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 4 commits into from
Feb 21, 2024

Conversation

mingmingl-llvm
Copy link
Contributor

@mingmingl-llvm mingmingl-llvm commented Feb 16, 2024

Without this, each time InstrProfData.inc is modified (like in #81691), pre-commit CI clang-format aggressively formats many lines in an unreadable way. Pull request with red pre-commit checks are unpleasant.

  • Use // clang-format:<reason> instead of /* clang-format */. The former allows specifying a reason but the latter is not.

@mingmingl-llvm mingmingl-llvm marked this pull request as ready for review February 20, 2024 22:22
@@ -62,6 +62,7 @@
#define INSTR_PROF_VISIBILITY
#endif

// clang-format off:re-enable clang-format after `issue #82426` is fixed
Copy link
Contributor

Choose a reason for hiding this comment

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

is there a reason in the future to reenable clang format for these macros?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The main motivation is to provide a context why clang-format is turned off.

If clang-format could format macros in a readable way, re-enabling it gives automated code formats.

Meanwhile, clang-format 18 introduces an option SkipMacroDefinitionBody (https://clang.llvm.org/docs/ClangFormatStyleOptions.html#skipmacrodefinitionbody) which seems relevant. However pre-commit CI currently uses clang format version 17, and afaik would emit hard error if .clang-format contains unparsable fields.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

reworded to explain the reason to turn off clang-format now (and when we might consider re-enable it). PTAL.

@mingmingl-llvm mingmingl-llvm merged commit 4247175 into main Feb 21, 2024
@mingmingl-llvm mingmingl-llvm deleted the users/minglotus-6/format branch February 21, 2024 18:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants