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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
