[clang-format] Correctly annotate designated initializer with PP if #65409
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
When encountering braces, such as those of a designated initializer, clang-format scans ahead to see what is contained within the braces. If it found a statement, like an if-statement of for-loop, it would deem the braces as not an initializer, but as a block instead. However, this heuristic incorrectly included a preprocessor `#if` line as an if-statement. This manifested in strange results and discrepancies between `#ifdef` and `#if defined`. With this patch, `if` is now ignored if it is preceeded by `#`. Fixes most of llvm#56685
This was referenced Sep 5, 2023
HazardyKnusperkeks
approved these changes
Sep 6, 2023
mydeveloperday
approved these changes
Sep 6, 2023
owenca
reviewed
Sep 7, 2023
Comment on lines
629
to
630
| if (PrevTok->is(tok::hash)) | ||
| break; |
There was a problem hiding this comment.
This should be applied to kw_if only, right? So something like the following instead?
case tok::identifier:
if (Tok->isNot(TT_StatementMacro))
break;
[[fallthrough]];
case tok::kw_if:
if (PrevTok->is(tok::hash))
break;
[[fallthrough]];
case tok::at:
case tok::semi:
case tok::kw_while:
case tok::kw_for:
case tok::kw_switch:
case tok::kw_try:
case tok::kw___try:
if (!LBraceStack.empty() && LBraceStack.back().Tok->is(BK_Unknown))
LBraceStack.back().Tok->setBlockKind(BK_Block);
break;
owenca
reviewed
Sep 7, 2023
Comment on lines
2014
to
2017
| "#ifdef FOO\n" | ||
| " .a = 1,\n" | ||
| "#endif\n" | ||
| " .b = 2\n" |
owenca
reviewed
Sep 7, 2023
Comment on lines
2025
to
2029
| "#if defined FOO\n" | ||
| " .a = 1,\n" | ||
| "#endif\n" | ||
| " .b = 2\n" | ||
| "};"); |
HazardyKnusperkeks
approved these changes
Sep 7, 2023
rymiel
added a commit
to rymiel/llvm-project
that referenced
this pull request
Oct 18, 2023
Pull request llvm#65409 changed the brace kind heuristic to not treat a preprocessor if directive as a in statement, however, this caused some regressions. If the contents of a brace don't immediately determine the brace kind, the heuristic will look at the characters immediately before and after the closing brace to determine the brace type. Unfortunately, if the closing brace was preceded by a preprocessor directive, for example `#endif`, the preceding token was `endif`, seen as just an identifier, so the braces were understood as a braced list. This patch fixes this behaviour by skipping all preprocessor directives when calculating brace types. Comments were already being skipped, so now preprocessor lines are skipped alongside comments. Fixes llvm#68404
rymiel
added a commit
that referenced
this pull request
Nov 3, 2023
Pull request #65409 changed the brace kind heuristic to not treat a preprocessor if directive as a in statement, however, this caused some regressions. If the contents of a brace don't immediately determine the brace kind, the heuristic will look at the characters immediately before and after the closing brace to determine the brace type. Unfortunately, if the closing brace was preceded by a preprocessor directive, for example `#endif`, the preceding token was `endif`, seen as just an identifier, so the braces were understood as a braced list. This patch fixes this behaviour by skipping all preprocessor directives when calculating brace types. Comments were already being skipped, so now preprocessor lines are skipped alongside comments. Fixes #68404
aarongable
pushed a commit
to chromium/chromium
that referenced
this pull request
Feb 14, 2024
LLVM pull no. 65409 [1] has largely fixed the underlying issue. In addition, MTECheckedPtr has been fully removed (crbug.com/40233676), no longer necessitating interspersing preprocessor gates with designated initializers. `git cl format` no longer does anything unexpected in these particular blocks. [1]: llvm/llvm-project#65409 Fixed: 40854198 Change-Id: Icd36b3dbb5cac24b7aacf63916e91ac5b9e125ef Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5294696 Auto-Submit: Kalvin Lee <[email protected]> Commit-Queue: Keishi Hattori <[email protected]> Reviewed-by: Keishi Hattori <[email protected]> Cr-Commit-Position: refs/heads/main@{#1260289}
github-actions bot
pushed a commit
to kaidokert/chrome_base_mirror
that referenced
this pull request
Feb 15, 2024
LLVM pull no. 65409 [1] has largely fixed the underlying issue. In addition, MTECheckedPtr has been fully removed (crbug.com/40233676), no longer necessitating interspersing preprocessor gates with designated initializers. `git cl format` no longer does anything unexpected in these particular blocks. [1]: llvm/llvm-project#65409 Fixed: 40854198 Change-Id: Icd36b3dbb5cac24b7aacf63916e91ac5b9e125ef Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5294696 Auto-Submit: Kalvin Lee <[email protected]> Commit-Queue: Keishi Hattori <[email protected]> Reviewed-by: Keishi Hattori <[email protected]> Cr-Commit-Position: refs/heads/main@{#1260289} NOKEYCHECK=True GitOrigin-RevId: fd0b9c3bbf560d709b3ebcd443efec8cc73afc68
jrguzman-ms
pushed a commit
to msft-mirror-aosp/platform.external.libchrome
that referenced
this pull request
Feb 15, 2024
LLVM pull no. 65409 [1] has largely fixed the underlying issue. In addition, MTECheckedPtr has been fully removed (crbug.com/40233676), no longer necessitating interspersing preprocessor gates with designated initializers. `git cl format` no longer does anything unexpected in these particular blocks. [1]: llvm/llvm-project#65409 Fixed: 40854198 Change-Id: Icd36b3dbb5cac24b7aacf63916e91ac5b9e125ef Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5294696 Auto-Submit: Kalvin Lee <[email protected]> Commit-Queue: Keishi Hattori <[email protected]> Reviewed-by: Keishi Hattori <[email protected]> Cr-Commit-Position: refs/heads/main@{#1260289} CrOS-Libchrome-Original-Commit: fd0b9c3bbf560d709b3ebcd443efec8cc73afc68
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
When encountering braces, such as those of a designated initializer, clang-format scans ahead to see what is contained within the braces. If it found a statement, like an if-statement of for-loop, it would deem the braces as not an initializer, but as a block instead.
However, this heuristic incorrectly included a preprocessor
#ifline as an if-statement. This manifested in strange results and discrepancies between#ifdefand#if defined.With this patch,
ifis now ignored if it is preceeded by#.Fixes most of #56685