Repository navigation
Conversation
|
Hello @ljkx 👋 Thank you for submitting a Pull Request (PR) to the LLVM Project. Since this is your first PR, here are a few useful links covering our main contribution policies and review practices.
Please reply to this message to confirm that you have read these policies, especially the LLVM AI Tool Use Policy, and that any AI tool usage has been noted in the PR description. Frequently asked questionsHow do I add reviewers? This PR will be automatically labeled, and the relevant teams will be notified. For some parts of the project, reviewers may also be added automatically. You can also add reviewers manually using the Reviewers section on this page. If you cannot use that section, it is probably because you do not have write permissions for the repository. In that case, you can request a review by tagging reviewers in a comment using What if there are no comments? If you have not received any comments on your PR after a week, you can request a review by pinging the PR with a comment such as “Ping”. The common courtesy ping rate is once a week. Please remember that you are asking for volunteer time from other developers. Are any special GitHub settings required to contribute to LLVM? We only require contributors to have a public email address associated with their GitHub commits, see this section of LLVM Developer Policy for details. If you have questions, feel free to leave a comment on this PR, or ask on LLVM Discord or LLVM Discourse. Thank you, |
|
@llvm/pr-subscribers-clang-tools-extra @llvm/pr-subscribers-clang-tidy Author: ljkx (ljkx) ChangesThe check found the last token of an enum by searching backward from the Find the last token by lexing forward from the opening brace instead. Fixes #237305. Assisted-by: Claude Code (claude-opus-5-5) 🤖 Generated with Claude Code Full diff: https://github.com/llvm/llvm-project/pull/241556.diff 6 Files Affected:
diff --git a/clang-tools-extra/clang-tidy/readability/TrailingCommaCheck.cpp b/clang-tools-extra/clang-tidy/readability/TrailingCommaCheck.cpp
index c0ed9849304de9..4a2741bc2a602c 100644
--- a/clang-tools-extra/clang-tidy/readability/TrailingCommaCheck.cpp
+++ b/clang-tools-extra/clang-tidy/readability/TrailingCommaCheck.cpp
@@ -122,9 +122,27 @@ void TrailingCommaCheck::checkEnumDecl(const EnumDecl *Enum,
if (Policy == CommaPolicyKind::Ignore)
return;
- const std::optional<Token> LastTok = Lexer::findPreviousToken(
- Enum->getBraceRange().getEnd(), *Result.SourceManager, getLangOpts(),
- /*IncludeComments=*/false);
+ // Lex forward from the opening brace rather than backward from the closing
+ // one: lexing backward restarts at the beginning of a line, which yields
+ // bogus tokens when that line is inside a multi-line block comment.
+ const SourceLocation RBraceLoc = Enum->getBraceRange().getEnd();
+ std::optional<Token> LastTok;
+ SourceLocation Loc = Enum->getBraceRange().getBegin();
+ while (true) {
+ const std::optional<Token> Tok =
+ Lexer::findNextToken(Loc, *Result.SourceManager, getLangOpts());
+ // The braces can be in different files, e.g. when the enum's body is
+ // split across an #include.
+ if (!Tok || Tok->is(tok::eof))
+ return;
+ if (Tok->getLocation() == RBraceLoc)
+ break;
+ if (!Result.SourceManager->isBeforeInTranslationUnit(Tok->getLocation(),
+ RBraceLoc))
+ return;
+ LastTok = Tok;
+ Loc = Tok->getLocation();
+ }
if (!LastTok)
return;
diff --git a/clang-tools-extra/docs/ReleaseNotes.md b/clang-tools-extra/docs/ReleaseNotes.md
index b6774370511feb..e7a55c3cce6118 100644
--- a/clang-tools-extra/docs/ReleaseNotes.md
+++ b/clang-tools-extra/docs/ReleaseNotes.md
@@ -473,6 +473,9 @@ infrastructure are described first, followed by tool-specific sections.
- Fixed a false positive on empty brace initializers of types with default
member initializers.
+ - Fixed a false positive and an invalid fix-it when an enum's last
+ enumerator is followed by a block comment that spans multiple lines.
+
- Improved {doc}`readability-use-std-min-max
<clang-tidy/checks/readability/use-std-min-max>` check by fixing spurious
trailing semicolons and lost comments when the `if` body has no braces.
diff --git a/clang-tools-extra/test/clang-tidy/checkers/readability/Inputs/trailing-comma/split-enum.h b/clang-tools-extra/test/clang-tidy/checkers/readability/Inputs/trailing-comma/split-enum.h
new file mode 100644
index 00000000000000..f7e0a32f07fa65
--- /dev/null
+++ b/clang-tools-extra/test/clang-tidy/checkers/readability/Inputs/trailing-comma/split-enum.h
@@ -0,0 +1,2 @@
+enum SplitEnum {
+ SplitA,
diff --git a/clang-tools-extra/test/clang-tidy/checkers/readability/trailing-comma-remove.cpp b/clang-tools-extra/test/clang-tidy/checkers/readability/trailing-comma-remove.cpp
index fadce1808876f7..27461572b1e47b 100644
--- a/clang-tools-extra/test/clang-tidy/checkers/readability/trailing-comma-remove.cpp
+++ b/clang-tools-extra/test/clang-tidy/checkers/readability/trailing-comma-remove.cpp
@@ -33,6 +33,16 @@ enum E3 {
};
enum Empty {};
+enum MultiLineComment {
+ MLC_A, /* comment that
+ spans multiple lines */
+};
+// CHECK-MESSAGES: :[[@LINE-3]]:8: warning: enum should not have a trailing comma
+// CHECK-FIXES: enum MultiLineComment {
+// CHECK-FIXES-NEXT: MLC_A /* comment that
+// CHECK-FIXES-NEXT: spans multiple lines */
+// CHECK-FIXES-NEXT: };
+
void f() {
// Multi-line init lists with trailing commas - should warn to remove
int a[] = {
diff --git a/clang-tools-extra/test/clang-tidy/checkers/readability/trailing-comma-split-enum.cpp b/clang-tools-extra/test/clang-tidy/checkers/readability/trailing-comma-split-enum.cpp
new file mode 100644
index 00000000000000..e46f0341209c02
--- /dev/null
+++ b/clang-tools-extra/test/clang-tidy/checkers/readability/trailing-comma-split-enum.cpp
@@ -0,0 +1,8 @@
+// RUN: %check_clang_tidy %s readability-trailing-comma %t -- \
+// RUN: -- -I %S/Inputs/trailing-comma
+
+// The enum's opening brace is in the header and its closing brace is here.
+// The check must not hang looking for the closing brace.
+#include "split-enum.h"
+ SplitB
+};
diff --git a/clang-tools-extra/test/clang-tidy/checkers/readability/trailing-comma.cpp b/clang-tools-extra/test/clang-tidy/checkers/readability/trailing-comma.cpp
index fb15931731b076..fc120002c6a67f 100644
--- a/clang-tools-extra/test/clang-tidy/checkers/readability/trailing-comma.cpp
+++ b/clang-tools-extra/test/clang-tidy/checkers/readability/trailing-comma.cpp
@@ -190,6 +190,21 @@ enum NullDirectiveAfter {
#
};
+enum MultiLineCommentAfterComma {
+ MLC_A, /* comment that
+ spans multiple lines */
+};
+
+enum MultiLineCommentNoComma {
+ MLC_B /* comment that
+ spans multiple lines */
+};
+// CHECK-MESSAGES: :[[@LINE-3]]:8: warning: enum should have a trailing comma
+// CHECK-FIXES: enum MultiLineCommentNoComma {
+// CHECK-FIXES-NEXT: MLC_B, /* comment that
+// CHECK-FIXES-NEXT: spans multiple lines */
+// CHECK-FIXES-NEXT: };
+
// Macros are ignored
#define ENUM(n, a, b) enum n { a, b }
#define INIT {1, 2}
|
127ad72 to
afb900e
Compare
The check found the last token of an enum by searching backward from the closing brace with Lexer::findPreviousToken. That restarts lexing at the beginning of the line, so when the closing brace follows a block comment spanning multiple lines, lexing starts inside the comment and the `/` of `*/` was taken as the last token. This produced wrong diagnostics and fix-its: with the Append policy a comma was inserted after the comment even when a trailing comma was already present, and with the Remove policy a trailing comma was not diagnosed. Find the last token by lexing forward to the closing brace instead, starting at the opening brace, or at the beginning of the closing brace's file if the braces are in different files. Fixes llvm#237305. Assisted-by: Claude Code
afb900e to
ac97c5b
Compare
| const auto [LBraceFile, LBraceOffset] = | ||
| SM.getDecomposedExpansionLoc(Enum->getBraceRange().getBegin()); | ||
| const auto [RBraceFile, RBraceOffset] = | ||
| SM.getDecomposedExpansionLoc(Enum->getBraceRange().getEnd()); |
There was a problem hiding this comment.
Does this also work for macro generated enums ?
There was a problem hiding this comment.
Yes. Macro handling is unchanged by this PR:
- An enum whose
enumkeyword or closing brace comes from a macro expansion is still skipped by the existingunless(isMacro())matcher, so this function never sees it. - If only the opening brace comes from a macro, or macros are used between the braces, the function takes the expansion locations of the braces and lexes the spelled tokens between them. That gives the same last token as
findPreviousTokendid.
To check, I compared this branch against a build of the same main commit without the patch, on a file of macro cases (enum from a function-like macro, enum inside a macro argument, enumerators wrapped in a macro, last enumerator from a macro, an X-macro list, opening or closing brace from a macro), with both Append and Remove as the multi-line policy. The diagnostics are identical except for the cases with a multi-line block comment before the closing brace, which is what this PR fixes.
I added tests for four of these cases in 0ed8801.
Three existing false positives showed up in that comparison. They are the same before and after, so I left them out of this PR: a macro that expands to enumerators with a trailing comma (#define ITEMS A, B,), an #undef X directly before the closing brace, and an empty macro after the trailing comma (B, EMPTY).
Assisted-by: Claude Code
|
I have read the contribution policies linked above, including the LLVM AI Tool Use Policy. AI tool usage is noted in the PR description ( |
The check found the last token of an enum by searching backward from the
closing brace with Lexer::findPreviousToken. That restarts lexing at the
beginning of the line, so when the closing brace follows a block comment
spanning multiple lines, lexing starts inside the comment and the
/of*/was taken as the last token. This produced wrong diagnostics andfix-its: with the Append policy a comma was inserted after the comment
even when a trailing comma was already present, and with the Remove
policy a trailing comma was not diagnosed.
Find the last token by lexing forward to the closing brace instead,
starting at the opening brace, or at the beginning of the closing brace's
file if the braces are in different files.
Fixes #237305.
Assisted-by: Claude Code