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

[Sam McCall via Phabricator via cfe-commits]

sammccall 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);
----------------
nit: inline?


================
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:
> nit: inline?
should this be the founddecl instead of the memberdecl?


================
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(),
----------------
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 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:
> 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


================
Comment at: clang-tools-extra/include-cleaner/lib/WalkAST.cpp:72
+
+  bool VisitUsingDecl(UsingDecl *UD) {
+    for (const auto *Shadow : UD->shadows())
----------------
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)


================
Comment at: clang-tools-extra/include-cleaner/lib/WalkAST.cpp:79
+  bool VisitFunctionDecl(FunctionDecl *FD) {
+    // Only mark the declaration from a definition.
+    if (FD->isThisDeclarationADefinition())
----------------
comment echoes the code, say why instead


================
Comment at: clang-tools-extra/include-cleaner/lib/WalkAST.cpp:110
+  bool VisitTemplateSpecializationTypeLoc(TemplateSpecializationTypeLoc TL) {
+    // FIXME: Handle specializations.
+    return handleTemplateName(TL.getTemplateNameLoc(),
----------------
nit: *explicit* specializations?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D132110