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

[Sam McCall via Phabricator via cfe-commits]

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:66
+  bool VisitOverloadExpr(OverloadExpr *E) {
+    // Mark all candidates as used when overload is not resolved.
+    llvm::for_each(E->decls(),
----------------
kadircet wrote:
> 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.
OK, add a FIXME?


================
Comment at: clang-tools-extra/include-cleaner/lib/WalkAST.cpp:72
+
+  bool VisitUsingDecl(UsingDecl *UD) {
+    for (const auto *Shadow : UD->shadows())
----------------
kadircet wrote:
> 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.
ack, again let's mark this as incomplete somehow (esp because this one is subtle)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D132110