[PATCH] D132110: [IncludeCleaner] Handle more C++ constructs

[Kadir Cetinkaya via Phabricator via cfe-commits]

kadircet added inline comments.


================
Comment at: clang-tools-extra/include-cleaner/lib/WalkAST.cpp:55
+  bool VisitMemberExpr(MemberExpr *E) {
+    auto *MD = E->getMemberDecl();
+    report(E->getMemberLoc(), MD);
----------------
sammccall wrote:
> sammccall wrote:
> > nit: inline?
> should this be the founddecl instead of the memberdecl?
right, forgot about these. also added a test.


================
Comment at: clang-tools-extra/include-cleaner/lib/WalkAST.cpp:66
+  bool VisitOverloadExpr(OverloadExpr *E) {
+    // Mark all candidates as used when overload is not resolved.
+    llvm::for_each(E->decls(),
----------------
sammccall wrote:
> sammccall wrote:
> > I think we need a policy knob for this, to decide whether to over or underestimate.
> > This would be the wrong behavior for missing-includes analysis.
> comment echoes the code, say why instead
i agree that this needs a knob. it's just unclear at which level currently, i am putting together a doc to have a better decision here (mostly to post vs pre filter).

i'd rather move forward with this version, to prepare grounds for the tidy check and clangd usage based on this library, and address these issues in a new iteration.


================
Comment at: clang-tools-extra/include-cleaner/lib/WalkAST.cpp:72
+
+  bool VisitUsingDecl(UsingDecl *UD) {
+    for (const auto *Shadow : UD->shadows())
----------------
sammccall wrote:
> I wonder if this is correct enough.
> 
> This brings a set of overloads into scope, the intention may be to bring a particular overload with others as harmless side effects: consider `using std::swap`.
> In this case, inserting includes for all the overloads that happened to be visible would be too much.
> 
> Possible behaviors:
>  - what we do here
>  - only do this if overestimate=true
>  - if overestimate=false, only include those USDs marked as `referenced` (not sure if they actually get marked appropriately, the bit exists)
i agree, basically the same things as I mentioned above, we should definitely have a way to filter these.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D132110/new/

https://reviews.llvm.org/D132110