[PATCH] D138779: [include-cleaner] Filter out references that not spelled in the main file.

[Kadir Cetinkaya via Phabricator via cfe-commits]

kadircet added a comment.

Maybe I am missing some context here, but why are we trying to filter out references outside of the main file?
ATM there's no concept of main file in neither walkUsed, it just receives some ast nodes for analyzing references to declarations inside these nodes, and the second use is directly figuring out what macro is referenced at particular locations, so I believe that filtering shouldn't happen for macros at all.
For decls, I can see some reasoning to filter out references that occur outside these AST nodes, rather than some notion of main file. Moreover we seem to be doing it in both users of walkAST, so maybe it would be better to do that there once instead?

This limiting the references to be under ast nodes behaviour has the nasty wrinkle of macros defined inside the main file, e.g:

  #define FOO foo()
  void bar() {
    FOO;
  }

we should say that `bar` uses `foo` despite the reference location lying outside of the AST node.

so maybe we should change the mapping policy from "just use spelling locations" to "use spelling location if it is in the same file as the expansion location and ignore the reference otherwise"?



================
Comment at: clang-tools-extra/include-cleaner/lib/Analysis.cpp:20
 
 void walkUsed(llvm::ArrayRef<Decl *> ASTRoots,
               llvm::ArrayRef<SymbolReference> MacroRefs,
----------------
as discussed offline we're introducing this extra constraint on references being provided, so let's mention that in the documentation as well. apart from that I agree with rest of the comments from Sam


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D138779