This is an archive of the discontinued LLVM Phabricator instance.

[include-cleaner] Handle base class member access from derived class.
ClosedPublic

Authored by VitaNuo on Dec 1 2022, 2:28 AM.

Diff Detail

Event Timeline

VitaNuo created this revision.Dec 1 2022, 2:28 AM
Herald added a project: Restricted Project. · View Herald TranscriptDec 1 2022, 2:28 AM
Herald added a subscriber: kadircet. · View Herald Transcript
VitaNuo requested review of this revision.Dec 1 2022, 2:28 AM
Herald added a project: Restricted Project. · View Herald TranscriptDec 1 2022, 2:28 AM
Herald added a subscriber: cfe-commits. · View Herald Transcript
VitaNuo updated this revision to Diff 479213.Dec 1 2022, 2:29 AM

Fix formatting.

hokein added inline comments.Dec 1 2022, 4:18 AM
clang-tools-extra/include-cleaner/lib/WalkAST.cpp
65

This is not safe, we can get a nullptr if this type is not a normal RecordType.

I think there are more cases we need to handle:

  1. pointer type (Derived *)
  2. reference type (Derived &)
  3. a dependent type

For the 3), some code like std::vector<T>().size(), it is tricky -- because we can't get any decl from a dependent type, we need some heuristics (in clangd we have some bits), we don't need to do that in this patch.

In the code implementation, we should add a dedicated handleType(QualType) helper to handle all these cases, so VisitMemberExpr, VisitCXXDependentScopeMemberExpr callbacks can just dispatch the type to this helper.

VitaNuo updated this revision to Diff 479319.Dec 1 2022, 8:53 AM

Handle pointer and reference types.

VitaNuo updated this revision to Diff 479339.Dec 1 2022, 9:53 AM

Refactor QualType to RecordDecl logic into a separate function.

kadircet added inline comments.Dec 2 2022, 1:24 AM
clang-tools-extra/include-cleaner/lib/WalkAST.cpp
62–68

comment doesn't really say why. maybe include something along the lines as we want to mark usage of most specific type (e.g. derived class, when using members from the base).

VitaNuo updated this revision to Diff 479556.Dec 2 2022, 1:42 AM

Add more comments. Add unimplemented VisitCXXDependentScopeMemberExpr method with a FIXME comment.

VitaNuo marked an inline comment as done.Dec 2 2022, 1:43 AM
VitaNuo added inline comments.
clang-tools-extra/include-cleaner/lib/WalkAST.cpp
65

Ok, added pointer types. Reference types seem to be working already. Added a FIXME for dependent members.

hokein added inline comments.Dec 2 2022, 7:13 AM
clang-tools-extra/include-cleaner/lib/WalkAST.cpp
68

nit: just report(E->getMemberLoc(), getDeclFromTye(Type));.

73

Be more specific about the comment, something like // FIMXE: support the dependent type case like "std::vector<T>().size();".

I will move this FIXME to VisitMemberExpr, and remove this function.

77
  • I'd use NamedDecl* as a return type (which is used in report)
  • move this method to private, it is only used internally inside class, no need to make it public
  • what do you think about naming it resolveType?
78

nit: remove the surrounding {}

81

nit: just return Type->getAsRecordDecl();.

VitaNuo marked 5 inline comments as done.Dec 6 2022, 1:17 AM
VitaNuo updated this revision to Diff 480378.Dec 6 2022, 1:18 AM

Address review comments, mostly style issues.

hokein added inline comments.Dec 6 2022, 3:20 AM
clang-tools-extra/include-cleaner/lib/WalkAST.cpp
62–68

I'd rephrase something like -- A member expr implies a usage of the class type (e.g. to prevent inserting a header of base class when using base members from a derived object).

clang-tools-extra/include-cleaner/unittests/WalkASTTest.cpp
182

can you add more test cases (the AST node is a bit different among the following cases) to make sure our code handle all of them?

  • Derived foo(); foo().^a;
  • Derived& foo(); foo().^a;
  • Derived().^a;
VitaNuo updated this revision to Diff 480422.Dec 6 2022, 3:39 AM
VitaNuo marked an inline comment as done.

Address review comments: change comment, add more test cases.

VitaNuo marked an inline comment as done.Dec 6 2022, 3:39 AM
hokein accepted this revision.Dec 8 2022, 2:07 AM

Thanks, looks good.

Add a Fix <github issue link> in the commit message.

clang-tools-extra/include-cleaner/lib/WalkAST.cpp
61

while reading it again, I realize that there is another case (operator overloading) we probably want to handle it as well. It is a more tricky case, no need to worry about it in this patch.

This revision is now accepted and ready to land.Dec 8 2022, 2:07 AM
VitaNuo added inline comments.Dec 8 2022, 2:36 AM
clang-tools-extra/include-cleaner/lib/WalkAST.cpp
61

Thank you. If you'd like me to take care of it, please file an issue with an example that needs to be taken care of :)