[PATCH] D148793: [clang-tidy] Implement an include-cleaner check.

[Viktoriia Bakalova via Phabricator via cfe-commits]

VitaNuo added a comment.

Thanks for the comments!



================
Comment at: clang-tools-extra/clang-tidy/misc/IncludeCleanerCheck.cpp:88
+    SourceLocation Loc = D->getLocation();
+    if (!SM->isWrittenInMainFile(SM->getSpellingLoc(Loc)))
+      continue;
----------------
hokein wrote:
> VitaNuo wrote:
> > hokein wrote:
> > > We should use the `getExpansionLoc` rather than the `SpellingLoc` here, otherwise we might miss the case like `TEST_F(WalkUsedTest, MultipleProviders) {... }` where the decl location is spelled in another file, but the function body is spelled in main-file.
> > > 
> > > we should actually use FileLoc of the decl location here (i.e. map it back to spelling location) as the decl might be introduced by a macro expansion, but if the spelling of "primary" location belongs to the main file we should still analyze it (e.g. decls introduced via `TEST` macros)
> > 
> > > we actually want spelling location, not fileloc
> > > sorry for the confusion
> > > basically, spelling location will always map to where a name is spelled in a          > physical file, even if it's part of macro body
> > > whereas getFileLoc, will map tokens from macro body to their expansion locations (i.e. place in a physical file where the macro is invoked)
> > 
> > These are earlier comments from Kadir on this topic. AFAIU we want the spelling location for `TEST_F(WalkUsedTest, MultipleProviders) {... }` because:
> > 
> > - for Decls introduced by the `TEST_F` expansion, we would like to analyze them only if they are spelled in the main file.
> > - for arguments, their transitive spelling location is where they're written. So if `TEST_F(WalkUsedTest, MultipleProviders) {... }` is written in the main file, the argument locations will be counted.
> > 
> I think I'm not convinced, see my comments below.
> 
> > for Decls introduced by the TEST_F expansion, we would like to analyze them only if they are spelled in the main file.
> 
> I think it is true for some cases. For example, a function decl has different parts (declarator, and function body), if the declarator is introduced by a macro which defined in a header, and the function body is spelled in the main-file, we still want to analyze the function body, see a simple example below:
> 
> ```
> // foo.h
> #define DECLARE(X) void X()
> 
> // main.cpp
> #include "foo.h"
> 
> DECLARE(myfunc) {
>    int a;
>    ...
> }
> ```
> 
> Moreover, we have use `ExpansionLoc` pattern in other places when we collect the main-file top-level decl ([include-cleaner](https://github.com/llvm/llvm-project/blob/main/clang-tools-extra/include-cleaner/lib/Record.cpp#L406), [clangd](https://github.com/llvm/llvm-project/blob/main/clang-tools-extra/clangd/SourceCode.cpp#L422)), and I don't see a special reason to change the pattern in the clang-tidy check.
Is the expansion location of `myfunc` in the main file? If that's the case, we need the expansion location indeed.
Otherwise, we need `getFileLoc` to map file locations from macro arguments to their spelling (in the main file above) and locations from macro bodies to the expansion location.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D148793