[PATCH] D158515: [include-cleaner] Make handling of enum constants similar to members
This revision was automatically updated to reflect the committed changes.
kadircet marked 2 inline comments as done.
Closed by commit rGab090e9e49ff: [include-cleaner] Make handling of enum
constants similar to members (authored by kadircet).
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D158515/new/
https://reviews.llvm.org/D158515
Files:
clang-tools-extra/include-cleaner/lib/WalkAST.cpp
clang-tools-extra/include-cleaner/unittests/WalkASTTest.cpp
Index: clang-tools-extra/include-cleaner/unittests/WalkASTTest.cpp
===
--- clang-tools-extra/include-cleaner/unittests/WalkASTTest.cpp
+++ clang-tools-extra/include-cleaner/unittests/WalkASTTest.cpp
@@ -50,7 +50,7 @@
Inputs.ExtraFiles["target.h"] = Target.code().str();
Inputs.ExtraArgs.push_back("-include");
Inputs.ExtraArgs.push_back("target.h");
- Inputs.ExtraArgs.push_back("-std=c++17");
+ Inputs.ExtraArgs.push_back("-std=c++20");
TestAST AST(Inputs);
const auto &SM = AST.sourceManager();
@@ -114,7 +114,7 @@
testWalk("int $explicit^x;", "int y = ^x;");
testWalk("int $explicit^foo();", "int y = ^foo();");
testWalk("namespace ns { int $explicit^x; }", "int y = ns::^x;");
- testWalk("struct $implicit^S { static int x; };", "int y = S::^x;");
+ testWalk("struct S { static int x; };", "int y = S::^x;");
// Canonical declaration only.
testWalk("extern int $explicit^x; int x;", "int y = ^x;");
// Return type of `foo` isn't used.
@@ -309,6 +309,14 @@
namespace ns { using ::$explicit^Foo; }
template<> struct ns::Foo {};)cpp",
"ns::^Foo x;");
+ testWalk(R"cpp(
+namespace ns { enum class foo { bar }; }
+using ns::foo;)cpp",
+ "auto x = foo::^bar;");
+ testWalk(R"cpp(
+namespace ns { enum foo { bar }; }
+using ns::foo::$explicit^bar;)cpp",
+ "auto x = ^bar;");
}
TEST(WalkAST, Using) {
@@ -338,6 +346,8 @@
}
using ns::$explicit^Y;)cpp",
"^Y x;");
+ testWalk("namespace ns { enum E {A}; } using enum ns::$explicit^E;",
+ "auto x = ^A;");
}
TEST(WalkAST, Namespaces) {
@@ -399,10 +409,10 @@
}
TEST(WalkAST, MemberExprs) {
- testWalk("struct $implicit^S { static int f; };", "void foo() { S::^f; }");
- testWalk("struct B { static int f; }; struct $implicit^S : B {};",
+ testWalk("struct S { static int f; };", "void foo() { S::^f; }");
+ testWalk("struct B { static int f; }; struct S : B {};",
"void foo() { S::^f; }");
- testWalk("struct B { static void f(); }; struct $implicit^S : B {};",
+ testWalk("struct B { static void f(); }; struct S : B {};",
"void foo() { S::^f; }");
testWalk("struct B { static void f(); }; ",
"struct S : B { void foo() { ^f(); } };");
Index: clang-tools-extra/include-cleaner/lib/WalkAST.cpp
===
--- clang-tools-extra/include-cleaner/lib/WalkAST.cpp
+++ clang-tools-extra/include-cleaner/lib/WalkAST.cpp
@@ -126,13 +126,22 @@
}
bool VisitDeclRefExpr(DeclRefExpr *DRE) {
-// Static class members are handled here, as they don't produce
MemberExprs.
-if (DRE->getFoundDecl()->isCXXClassMember()) {
- if (auto *Qual = DRE->getQualifier())
-report(DRE->getLocation(), Qual->getAsRecordDecl(), RefType::Implicit);
-} else {
- report(DRE->getLocation(), DRE->getFoundDecl());
+auto *FD = DRE->getFoundDecl();
+// For refs to non-meber-like decls, use the found decl.
+// For member-like decls, we should have a reference from the qualifier to
+// the container decl instead, which is preferred as it'll handle
+// aliases/exports properly.
+if (!FD->isCXXClassMember() && !llvm::isa(FD)) {
+ report(DRE->getLocation(), FD);
+ return true;
}
+// If the ref is without a qualifier, and is a member, ignore it. As it is
+// available in current context due to some other construct (e.g. base
+// specifiers, using decls) that has to spell the name explicitly.
+// If it's an enum constant, it must be due to prior decl. Report
references
+// to it instead.
+if (llvm::isa(FD) && !DRE->hasQualifier())
+ report(DRE->getLocation(), FD);
return true;
}
Index: clang-tools-extra/include-cleaner/unittests/WalkASTTest.cpp
===
--- clang-tools-extra/include-cleaner/unittests/WalkASTTest.cpp
+++ clang-tools-extra/include-cleaner/unittests/WalkASTTest.cpp
@@ -50,7 +50,7 @@
Inputs.ExtraFiles["target.h"] = Target.code().str();
Inputs.ExtraArgs.push_back("-include");
Inputs.ExtraArgs.push_back("target.h");
- Inputs.ExtraArgs.push_back("-std=c++17");
+ Inputs.ExtraArgs.push_back("-std=c++20");
TestAST AST(Inputs);
const auto &SM = AST.sourceManager();
@@ -114,7 +114,7 @@
testWalk("int $explicit^x;", "int y = ^x;");
testWalk("int $expl
[PATCH] D158515: [include-cleaner] Make handling of enum constants similar to members
kadircet updated this revision to Diff 552392.
kadircet added a comment.
- Change to c++20
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D158515/new/
https://reviews.llvm.org/D158515
Files:
clang-tools-extra/include-cleaner/lib/WalkAST.cpp
clang-tools-extra/include-cleaner/unittests/WalkASTTest.cpp
Index: clang-tools-extra/include-cleaner/unittests/WalkASTTest.cpp
===
--- clang-tools-extra/include-cleaner/unittests/WalkASTTest.cpp
+++ clang-tools-extra/include-cleaner/unittests/WalkASTTest.cpp
@@ -50,7 +50,7 @@
Inputs.ExtraFiles["target.h"] = Target.code().str();
Inputs.ExtraArgs.push_back("-include");
Inputs.ExtraArgs.push_back("target.h");
- Inputs.ExtraArgs.push_back("-std=c++17");
+ Inputs.ExtraArgs.push_back("-std=c++20");
TestAST AST(Inputs);
const auto &SM = AST.sourceManager();
@@ -114,7 +114,7 @@
testWalk("int $explicit^x;", "int y = ^x;");
testWalk("int $explicit^foo();", "int y = ^foo();");
testWalk("namespace ns { int $explicit^x; }", "int y = ns::^x;");
- testWalk("struct $implicit^S { static int x; };", "int y = S::^x;");
+ testWalk("struct S { static int x; };", "int y = S::^x;");
// Canonical declaration only.
testWalk("extern int $explicit^x; int x;", "int y = ^x;");
// Return type of `foo` isn't used.
@@ -309,6 +309,14 @@
namespace ns { using ::$explicit^Foo; }
template<> struct ns::Foo {};)cpp",
"ns::^Foo x;");
+ testWalk(R"cpp(
+namespace ns { enum class foo { bar }; }
+using ns::foo;)cpp",
+ "auto x = foo::^bar;");
+ testWalk(R"cpp(
+namespace ns { enum foo { bar }; }
+using ns::foo::$explicit^bar;)cpp",
+ "auto x = ^bar;");
}
TEST(WalkAST, Using) {
@@ -338,6 +346,8 @@
}
using ns::$explicit^Y;)cpp",
"^Y x;");
+ testWalk("namespace ns { enum E {A}; } using enum ns::$explicit^E;",
+ "auto x = ^A;");
}
TEST(WalkAST, Namespaces) {
@@ -399,10 +409,10 @@
}
TEST(WalkAST, MemberExprs) {
- testWalk("struct $implicit^S { static int f; };", "void foo() { S::^f; }");
- testWalk("struct B { static int f; }; struct $implicit^S : B {};",
+ testWalk("struct S { static int f; };", "void foo() { S::^f; }");
+ testWalk("struct B { static int f; }; struct S : B {};",
"void foo() { S::^f; }");
- testWalk("struct B { static void f(); }; struct $implicit^S : B {};",
+ testWalk("struct B { static void f(); }; struct S : B {};",
"void foo() { S::^f; }");
testWalk("struct B { static void f(); }; ",
"struct S : B { void foo() { ^f(); } };");
Index: clang-tools-extra/include-cleaner/lib/WalkAST.cpp
===
--- clang-tools-extra/include-cleaner/lib/WalkAST.cpp
+++ clang-tools-extra/include-cleaner/lib/WalkAST.cpp
@@ -126,13 +126,22 @@
}
bool VisitDeclRefExpr(DeclRefExpr *DRE) {
-// Static class members are handled here, as they don't produce
MemberExprs.
-if (DRE->getFoundDecl()->isCXXClassMember()) {
- if (auto *Qual = DRE->getQualifier())
-report(DRE->getLocation(), Qual->getAsRecordDecl(), RefType::Implicit);
-} else {
- report(DRE->getLocation(), DRE->getFoundDecl());
+auto *FD = DRE->getFoundDecl();
+// For refs to non-meber-like decls, use the found decl.
+// For member-like decls, we should have a reference from the qualifier to
+// the container decl instead, which is preferred as it'll handle
+// aliases/exports properly.
+if (!FD->isCXXClassMember() && !llvm::isa(FD)) {
+ report(DRE->getLocation(), FD);
+ return true;
}
+// If the ref is without a qualifier, and is a member, ignore it. As it is
+// available in current context due to some other construct (e.g. base
+// specifiers, using decls) that has to spell the name explicitly.
+// If it's an enum constant, it must be due to prior decl. Report
references
+// to it instead.
+if (llvm::isa(FD) && !DRE->hasQualifier())
+ report(DRE->getLocation(), FD);
return true;
}
Index: clang-tools-extra/include-cleaner/unittests/WalkASTTest.cpp
===
--- clang-tools-extra/include-cleaner/unittests/WalkASTTest.cpp
+++ clang-tools-extra/include-cleaner/unittests/WalkASTTest.cpp
@@ -50,7 +50,7 @@
Inputs.ExtraFiles["target.h"] = Target.code().str();
Inputs.ExtraArgs.push_back("-include");
Inputs.ExtraArgs.push_back("target.h");
- Inputs.ExtraArgs.push_back("-std=c++17");
+ Inputs.ExtraArgs.push_back("-std=c++20");
TestAST AST(Inputs);
const auto &SM = AST.sourceManager();
@@ -114,7 +114,7 @@
testWalk("int $explicit^x;", "int y = ^x;");
testWalk("int $explicit^foo();", "int y = ^foo();");
testWalk("namespace ns { int $explicit^x; }", "int y = ns::^x;");
- testWalk("struct $implicit^S { static int x;
[PATCH] D158515: [include-cleaner] Make handling of enum constants similar to members
kadircet updated this revision to Diff 552391.
kadircet added a comment.
- Add test for using enums
- Drop implicit references from qualified names to their containers, as we
should already have explicit references from the spelling of the qualifier.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D158515/new/
https://reviews.llvm.org/D158515
Files:
clang-tools-extra/include-cleaner/lib/WalkAST.cpp
clang-tools-extra/include-cleaner/unittests/WalkASTTest.cpp
Index: clang-tools-extra/include-cleaner/unittests/WalkASTTest.cpp
===
--- clang-tools-extra/include-cleaner/unittests/WalkASTTest.cpp
+++ clang-tools-extra/include-cleaner/unittests/WalkASTTest.cpp
@@ -114,7 +114,7 @@
testWalk("int $explicit^x;", "int y = ^x;");
testWalk("int $explicit^foo();", "int y = ^foo();");
testWalk("namespace ns { int $explicit^x; }", "int y = ns::^x;");
- testWalk("struct $implicit^S { static int x; };", "int y = S::^x;");
+ testWalk("struct S { static int x; };", "int y = S::^x;");
// Canonical declaration only.
testWalk("extern int $explicit^x; int x;", "int y = ^x;");
// Return type of `foo` isn't used.
@@ -309,6 +309,14 @@
namespace ns { using ::$explicit^Foo; }
template<> struct ns::Foo {};)cpp",
"ns::^Foo x;");
+ testWalk(R"cpp(
+namespace ns { enum class foo { bar }; }
+using ns::foo;)cpp",
+ "auto x = foo::^bar;");
+ testWalk(R"cpp(
+namespace ns { enum foo { bar }; }
+using ns::foo::$explicit^bar;)cpp",
+ "auto x = ^bar;");
}
TEST(WalkAST, Using) {
@@ -338,6 +346,8 @@
}
using ns::$explicit^Y;)cpp",
"^Y x;");
+ testWalk("namespace ns { enum E {A}; } using enum ns::$explicit^E;",
+ "auto x = ^A;");
}
TEST(WalkAST, Namespaces) {
@@ -399,10 +409,10 @@
}
TEST(WalkAST, MemberExprs) {
- testWalk("struct $implicit^S { static int f; };", "void foo() { S::^f; }");
- testWalk("struct B { static int f; }; struct $implicit^S : B {};",
+ testWalk("struct S { static int f; };", "void foo() { S::^f; }");
+ testWalk("struct B { static int f; }; struct S : B {};",
"void foo() { S::^f; }");
- testWalk("struct B { static void f(); }; struct $implicit^S : B {};",
+ testWalk("struct B { static void f(); }; struct S : B {};",
"void foo() { S::^f; }");
testWalk("struct B { static void f(); }; ",
"struct S : B { void foo() { ^f(); } };");
Index: clang-tools-extra/include-cleaner/lib/WalkAST.cpp
===
--- clang-tools-extra/include-cleaner/lib/WalkAST.cpp
+++ clang-tools-extra/include-cleaner/lib/WalkAST.cpp
@@ -126,13 +126,22 @@
}
bool VisitDeclRefExpr(DeclRefExpr *DRE) {
-// Static class members are handled here, as they don't produce
MemberExprs.
-if (DRE->getFoundDecl()->isCXXClassMember()) {
- if (auto *Qual = DRE->getQualifier())
-report(DRE->getLocation(), Qual->getAsRecordDecl(), RefType::Implicit);
-} else {
- report(DRE->getLocation(), DRE->getFoundDecl());
+auto *FD = DRE->getFoundDecl();
+// For refs to non-meber-like decls, use the found decl.
+// For member-like decls, we should have a reference from the qualifier to
+// the container decl instead, which is preferred as it'll handle
+// aliases/exports properly.
+if (!FD->isCXXClassMember() && !llvm::isa(FD)) {
+ report(DRE->getLocation(), FD);
+ return true;
}
+// If the ref is without a qualifier, and is a member, ignore it. As it is
+// available in current context due to some other construct (e.g. base
+// specifiers, using decls) that has to spell the name explicitly.
+// If it's an enum constant, it must be due to prior decl. Report
references
+// to it instead.
+if (llvm::isa(FD) && !DRE->hasQualifier())
+ report(DRE->getLocation(), FD);
return true;
}
Index: clang-tools-extra/include-cleaner/unittests/WalkASTTest.cpp
===
--- clang-tools-extra/include-cleaner/unittests/WalkASTTest.cpp
+++ clang-tools-extra/include-cleaner/unittests/WalkASTTest.cpp
@@ -114,7 +114,7 @@
testWalk("int $explicit^x;", "int y = ^x;");
testWalk("int $explicit^foo();", "int y = ^foo();");
testWalk("namespace ns { int $explicit^x; }", "int y = ns::^x;");
- testWalk("struct $implicit^S { static int x; };", "int y = S::^x;");
+ testWalk("struct S { static int x; };", "int y = S::^x;");
// Canonical declaration only.
testWalk("extern int $explicit^x; int x;", "int y = ^x;");
// Return type of `foo` isn't used.
@@ -309,6 +309,14 @@
namespace ns { using ::$explicit^Foo; }
template<> struct ns::Foo {};)cpp",
"ns::^Foo x;");
+ testWalk(R"cpp(
+namespace ns { enum class foo { bar }; }
+using ns::foo;)cpp",
+ "auto x = foo:
[PATCH] D158515: [include-cleaner] Make handling of enum constants similar to members
sammccall accepted this revision.
sammccall added inline comments.
This revision is now accepted and ready to land.
Comment at: clang-tools-extra/include-cleaner/lib/WalkAST.cpp:135
+}
+// For refs to member-like decls, report an implicit ref to the container.
+if (auto *Qual = DRE->getQualifier()) {
try to write a test case that relies on this :-)
Comment at: clang-tools-extra/include-cleaner/unittests/WalkASTTest.cpp:320
+ "auto x = ^bar;");
}
test using enum
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D158515/new/
https://reviews.llvm.org/D158515
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D158515: [include-cleaner] Make handling of enum constants similar to members
kadircet created this revision.
kadircet added a reviewer: sammccall.
Herald added a project: All.
kadircet requested review of this revision.
Herald added a project: clang-tools-extra.
Herald added a subscriber: cfe-commits.
We were treating enum constants more like regular decls, which results
in ignoring type aliases/exports.
This patch brings the handling to be closer to member-like decls, with
one caveat. When we encounter reference to an enum constant we still
report an explicit reference to the particular enum constant, as
otherwise we might not see any references to the enum itself.
Repository:
rG LLVM Github Monorepo
https://reviews.llvm.org/D158515
Files:
clang-tools-extra/include-cleaner/lib/WalkAST.cpp
clang-tools-extra/include-cleaner/unittests/WalkASTTest.cpp
Index: clang-tools-extra/include-cleaner/unittests/WalkASTTest.cpp
===
--- clang-tools-extra/include-cleaner/unittests/WalkASTTest.cpp
+++ clang-tools-extra/include-cleaner/unittests/WalkASTTest.cpp
@@ -309,6 +309,14 @@
namespace ns { using ::$explicit^Foo; }
template<> struct ns::Foo {};)cpp",
"ns::^Foo x;");
+ testWalk(R"cpp(
+namespace ns { enum class foo { bar }; }
+using ns::$implicit^foo;)cpp",
+ "auto x = foo::^bar;");
+ testWalk(R"cpp(
+namespace ns { enum foo { bar }; }
+using ns::foo::$explicit^bar;)cpp",
+ "auto x = ^bar;");
}
TEST(WalkAST, Using) {
Index: clang-tools-extra/include-cleaner/lib/WalkAST.cpp
===
--- clang-tools-extra/include-cleaner/lib/WalkAST.cpp
+++ clang-tools-extra/include-cleaner/lib/WalkAST.cpp
@@ -126,13 +126,27 @@
}
bool VisitDeclRefExpr(DeclRefExpr *DRE) {
-// Static class members are handled here, as they don't produce
MemberExprs.
-if (DRE->getFoundDecl()->isCXXClassMember()) {
- if (auto *Qual = DRE->getQualifier())
-report(DRE->getLocation(), Qual->getAsRecordDecl(), RefType::Implicit);
-} else {
- report(DRE->getLocation(), DRE->getFoundDecl());
+auto *FD = DRE->getFoundDecl();
+// For refs to non-meber-like decls, use the found decl.
+if (!FD->isCXXClassMember() && !llvm::isa(FD)) {
+ report(DRE->getLocation(), FD);
+ return true;
+}
+// For refs to member-like decls, report an implicit ref to the container.
+if (auto *Qual = DRE->getQualifier()) {
+ if (auto *Ty = Qual->getAsType()) {
+report(DRE->getLocation(), getMemberProvider(QualType(Ty, 0)),
+ RefType::Implicit);
+ }
+ return true;
}
+// If the ref is without a qualifier, and is a member, ignore it. As it is
+// available in current context due to some other construct (e.g. base
+// specifiers, using decls) that has to spell the name explicitly.
+// If it's an enum constant, it must be due to prior decl. Report
references
+// to it instead.
+if (llvm::isa(FD))
+ report(DRE->getLocation(), FD);
return true;
}
Index: clang-tools-extra/include-cleaner/unittests/WalkASTTest.cpp
===
--- clang-tools-extra/include-cleaner/unittests/WalkASTTest.cpp
+++ clang-tools-extra/include-cleaner/unittests/WalkASTTest.cpp
@@ -309,6 +309,14 @@
namespace ns { using ::$explicit^Foo; }
template<> struct ns::Foo {};)cpp",
"ns::^Foo x;");
+ testWalk(R"cpp(
+namespace ns { enum class foo { bar }; }
+using ns::$implicit^foo;)cpp",
+ "auto x = foo::^bar;");
+ testWalk(R"cpp(
+namespace ns { enum foo { bar }; }
+using ns::foo::$explicit^bar;)cpp",
+ "auto x = ^bar;");
}
TEST(WalkAST, Using) {
Index: clang-tools-extra/include-cleaner/lib/WalkAST.cpp
===
--- clang-tools-extra/include-cleaner/lib/WalkAST.cpp
+++ clang-tools-extra/include-cleaner/lib/WalkAST.cpp
@@ -126,13 +126,27 @@
}
bool VisitDeclRefExpr(DeclRefExpr *DRE) {
-// Static class members are handled here, as they don't produce MemberExprs.
-if (DRE->getFoundDecl()->isCXXClassMember()) {
- if (auto *Qual = DRE->getQualifier())
-report(DRE->getLocation(), Qual->getAsRecordDecl(), RefType::Implicit);
-} else {
- report(DRE->getLocation(), DRE->getFoundDecl());
+auto *FD = DRE->getFoundDecl();
+// For refs to non-meber-like decls, use the found decl.
+if (!FD->isCXXClassMember() && !llvm::isa(FD)) {
+ report(DRE->getLocation(), FD);
+ return true;
+}
+// For refs to member-like decls, report an implicit ref to the container.
+if (auto *Qual = DRE->getQualifier()) {
+ if (auto *Ty = Qual->getAsType()) {
+report(DRE->getLocation(), getMemberProvider(QualType(Ty, 0)),
+ RefType::Implicit);
+ }
+ return true;
}
+// If the ref is without a qualifier, and is a
