[clang-tidy] Do not emit file path for anonymous enums in readability-enum-initial-value check
#112496
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
|
@llvm/pr-subscribers-clang-tools-extra Author: Discookie (Discookie) ChangesPreviously the name of anonymous enums in the check were We should think about removing emitted file paths globally from clang-tidy as well, if appropriate. Or at least removing the file paths from all locations where the warning/note tag already points to the declaration. Full diff: https://github.com/llvm/llvm-project/pull/112496.diff 3 Files Affected:
diff --git a/clang-tools-extra/clang-tidy/readability/EnumInitialValueCheck.cpp b/clang-tools-extra/clang-tidy/readability/EnumInitialValueCheck.cpp
index 1cb95c2b2347b7..5a48b103b9d602 100644
--- a/clang-tools-extra/clang-tidy/readability/EnumInitialValueCheck.cpp
+++ b/clang-tools-extra/clang-tidy/readability/EnumInitialValueCheck.cpp
@@ -123,6 +123,13 @@ AST_MATCHER(EnumDecl, hasSequentialInitialValues) {
return !AllEnumeratorsArePowersOfTwo;
}
+std::string getName(const EnumDecl *Decl) {
+ if (!Decl->getDeclName())
+ return "<unnamed>";
+
+ return Decl->getQualifiedNameAsString();
+}
+
} // namespace
EnumInitialValueCheck::EnumInitialValueCheck(StringRef Name,
@@ -158,12 +165,15 @@ void EnumInitialValueCheck::registerMatchers(MatchFinder *Finder) {
}
void EnumInitialValueCheck::check(const MatchFinder::MatchResult &Result) {
+ PrintingPolicy PP = Result.Context->getPrintingPolicy();
+ PP.AnonymousTagLocations = false;
+
if (const auto *Enum = Result.Nodes.getNodeAs<EnumDecl>("inconsistent")) {
DiagnosticBuilder Diag =
diag(Enum->getBeginLoc(),
- "initial values in enum %0 are not consistent, consider explicit "
+ "initial values in enum '%0' are not consistent, consider explicit "
"initialization of all, none or only the first enumerator")
- << Enum;
+ << getName(Enum);
for (const EnumConstantDecl *ECD : Enum->enumerators())
if (ECD->getInitExpr() == nullptr) {
const SourceLocation EndLoc = Lexer::getLocForEndOfToken(
@@ -183,16 +193,16 @@ void EnumInitialValueCheck::check(const MatchFinder::MatchResult &Result) {
if (Loc.isInvalid() || Loc.isMacroID())
return;
DiagnosticBuilder Diag = diag(Loc, "zero initial value for the first "
- "enumerator in %0 can be disregarded")
- << Enum;
+ "enumerator in '%0' can be disregarded")
+ << getName(Enum);
cleanInitialValue(Diag, ECD, *Result.SourceManager, getLangOpts());
return;
}
if (const auto *Enum = Result.Nodes.getNodeAs<EnumDecl>("sequential")) {
DiagnosticBuilder Diag =
diag(Enum->getBeginLoc(),
- "sequential initial value in %0 can be ignored")
- << Enum;
+ "sequential initial value in '%0' can be ignored")
+ << getName(Enum);
for (const EnumConstantDecl *ECD : llvm::drop_begin(Enum->enumerators()))
cleanInitialValue(Diag, ECD, *Result.SourceManager, getLangOpts());
return;
diff --git a/clang-tools-extra/docs/ReleaseNotes.rst b/clang-tools-extra/docs/ReleaseNotes.rst
index 95be0a89cd6c93..a4427ac222b475 100644
--- a/clang-tools-extra/docs/ReleaseNotes.rst
+++ b/clang-tools-extra/docs/ReleaseNotes.rst
@@ -232,7 +232,8 @@ Changes in existing checks
- Improved :doc:`readability-enum-initial-value
<clang-tidy/checks/readability/enum-initial-value>` check by only issuing
- diagnostics for the definition of an ``enum``, and by fixing a typo in the
+ diagnostics for the definition of an ``enum``, by not emitting a redundant
+ file path for anonymous enums in the diagnostic, and by fixing a typo in the
diagnostic.
- Improved :doc:`readability-implicit-bool-conversion
diff --git a/clang-tools-extra/test/clang-tidy/checkers/readability/enum-initial-value.c b/clang-tools-extra/test/clang-tidy/checkers/readability/enum-initial-value.c
index b9a34d0683d7f3..54108585f030f8 100644
--- a/clang-tools-extra/test/clang-tidy/checkers/readability/enum-initial-value.c
+++ b/clang-tools-extra/test/clang-tidy/checkers/readability/enum-initial-value.c
@@ -53,6 +53,17 @@ enum EMacro2 {
// CHECK-FIXES: EMacro2_c = 3,
};
+
+enum {
+ // CHECK-MESSAGES: :[[@LINE-1]]:1: warning: initial values in enum '<unnamed>' are not consistent
+ // CHECK-MESSAGES-ENABLE: :[[@LINE-2]]:1: warning: initial values in enum '<unnamed>' are not consistent
+ EAnonymous_a = 1,
+ EAnonymous_b,
+ // CHECK-FIXES: EAnonymous_b = 2,
+ EAnonymous_c = 3,
+};
+
+
enum EnumZeroFirstInitialValue {
EnumZeroFirstInitialValue_0 = 0,
// CHECK-MESSAGES-ENABLE: :[[@LINE-1]]:3: warning: zero initial value for the first enumerator in 'EnumZeroFirstInitialValue' can be disregarded
@@ -114,4 +125,3 @@ enum WithFwdDeclSequential : int {
EFS2 = 4,
// CHECK-FIXES-ENABLE: EFS2 ,
};
-
|
|
@llvm/pr-subscribers-clang-tidy Author: Discookie (Discookie) ChangesPreviously the name of anonymous enums in the check were We should think about removing emitted file paths globally from clang-tidy as well, if appropriate. Or at least removing the file paths from all locations where the warning/note tag already points to the declaration. Full diff: https://github.com/llvm/llvm-project/pull/112496.diff 3 Files Affected:
diff --git a/clang-tools-extra/clang-tidy/readability/EnumInitialValueCheck.cpp b/clang-tools-extra/clang-tidy/readability/EnumInitialValueCheck.cpp
index 1cb95c2b2347b7..5a48b103b9d602 100644
--- a/clang-tools-extra/clang-tidy/readability/EnumInitialValueCheck.cpp
+++ b/clang-tools-extra/clang-tidy/readability/EnumInitialValueCheck.cpp
@@ -123,6 +123,13 @@ AST_MATCHER(EnumDecl, hasSequentialInitialValues) {
return !AllEnumeratorsArePowersOfTwo;
}
+std::string getName(const EnumDecl *Decl) {
+ if (!Decl->getDeclName())
+ return "<unnamed>";
+
+ return Decl->getQualifiedNameAsString();
+}
+
} // namespace
EnumInitialValueCheck::EnumInitialValueCheck(StringRef Name,
@@ -158,12 +165,15 @@ void EnumInitialValueCheck::registerMatchers(MatchFinder *Finder) {
}
void EnumInitialValueCheck::check(const MatchFinder::MatchResult &Result) {
+ PrintingPolicy PP = Result.Context->getPrintingPolicy();
+ PP.AnonymousTagLocations = false;
+
if (const auto *Enum = Result.Nodes.getNodeAs<EnumDecl>("inconsistent")) {
DiagnosticBuilder Diag =
diag(Enum->getBeginLoc(),
- "initial values in enum %0 are not consistent, consider explicit "
+ "initial values in enum '%0' are not consistent, consider explicit "
"initialization of all, none or only the first enumerator")
- << Enum;
+ << getName(Enum);
for (const EnumConstantDecl *ECD : Enum->enumerators())
if (ECD->getInitExpr() == nullptr) {
const SourceLocation EndLoc = Lexer::getLocForEndOfToken(
@@ -183,16 +193,16 @@ void EnumInitialValueCheck::check(const MatchFinder::MatchResult &Result) {
if (Loc.isInvalid() || Loc.isMacroID())
return;
DiagnosticBuilder Diag = diag(Loc, "zero initial value for the first "
- "enumerator in %0 can be disregarded")
- << Enum;
+ "enumerator in '%0' can be disregarded")
+ << getName(Enum);
cleanInitialValue(Diag, ECD, *Result.SourceManager, getLangOpts());
return;
}
if (const auto *Enum = Result.Nodes.getNodeAs<EnumDecl>("sequential")) {
DiagnosticBuilder Diag =
diag(Enum->getBeginLoc(),
- "sequential initial value in %0 can be ignored")
- << Enum;
+ "sequential initial value in '%0' can be ignored")
+ << getName(Enum);
for (const EnumConstantDecl *ECD : llvm::drop_begin(Enum->enumerators()))
cleanInitialValue(Diag, ECD, *Result.SourceManager, getLangOpts());
return;
diff --git a/clang-tools-extra/docs/ReleaseNotes.rst b/clang-tools-extra/docs/ReleaseNotes.rst
index 95be0a89cd6c93..a4427ac222b475 100644
--- a/clang-tools-extra/docs/ReleaseNotes.rst
+++ b/clang-tools-extra/docs/ReleaseNotes.rst
@@ -232,7 +232,8 @@ Changes in existing checks
- Improved :doc:`readability-enum-initial-value
<clang-tidy/checks/readability/enum-initial-value>` check by only issuing
- diagnostics for the definition of an ``enum``, and by fixing a typo in the
+ diagnostics for the definition of an ``enum``, by not emitting a redundant
+ file path for anonymous enums in the diagnostic, and by fixing a typo in the
diagnostic.
- Improved :doc:`readability-implicit-bool-conversion
diff --git a/clang-tools-extra/test/clang-tidy/checkers/readability/enum-initial-value.c b/clang-tools-extra/test/clang-tidy/checkers/readability/enum-initial-value.c
index b9a34d0683d7f3..54108585f030f8 100644
--- a/clang-tools-extra/test/clang-tidy/checkers/readability/enum-initial-value.c
+++ b/clang-tools-extra/test/clang-tidy/checkers/readability/enum-initial-value.c
@@ -53,6 +53,17 @@ enum EMacro2 {
// CHECK-FIXES: EMacro2_c = 3,
};
+
+enum {
+ // CHECK-MESSAGES: :[[@LINE-1]]:1: warning: initial values in enum '<unnamed>' are not consistent
+ // CHECK-MESSAGES-ENABLE: :[[@LINE-2]]:1: warning: initial values in enum '<unnamed>' are not consistent
+ EAnonymous_a = 1,
+ EAnonymous_b,
+ // CHECK-FIXES: EAnonymous_b = 2,
+ EAnonymous_c = 3,
+};
+
+
enum EnumZeroFirstInitialValue {
EnumZeroFirstInitialValue_0 = 0,
// CHECK-MESSAGES-ENABLE: :[[@LINE-1]]:3: warning: zero initial value for the first enumerator in 'EnumZeroFirstInitialValue' can be disregarded
@@ -114,4 +125,3 @@ enum WithFwdDeclSequential : int {
EFS2 = 4,
// CHECK-FIXES-ENABLE: EFS2 ,
};
-
|
|
✅ With the latest revision this PR passed the C/C++ code formatter. |
…y-enum-initial-value` check
Discookie
force-pushed
the
enum-name-fix
branch
from
be0bee7 to
93c933b
Compare
EugeneZelenko
requested review from
PiotrZSL,
5chmidti,
SimplyDanny and
HerrCai0907
NagyDonat
approved these changes
Oct 24, 2024
There was a problem hiding this comment.
Looks good to me, this is an important improvement of report quality.
If the analysis is performed in a cloud-based environment (which is common), the full path name of the analyzed file may contain e.g. directory names that are unpredictably changing between runs, which seriously hinders comparing the results between different runs.
My only remark is that it would be more natural to use unnamed enum instead of the current enum '<unnamed>' (but this is not a blocking issue).
5chmidti
approved these changes
Oct 26, 2024
|
Removed a leftover PrintingPolicy call before merging. |
NoumanAmir657
pushed a commit
to NoumanAmir657/llvm-project
that referenced
this pull request
Nov 4, 2024
…y-enum-initial-value` check (llvm#112496) Previously the name of anonymous enums in the check were `enum 'enum (unnamed at /full/path/to/file.c:1:1)'`, which breaks reproducibility of clang-tidy reports when the analyzed project is in a different folder.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Previously the name of anonymous enums in the check were
enum 'enum (unnamed at /full/path/to/file.c:1:1)', which breaks reproducibility of clang-tidy reports when the analyzed project is in a different folder.We should think about removing emitted file paths globally from clang-tidy as well, if appropriate. Or at least removing the file paths from all locations where the warning/note tag already points to the declaration.