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