hokein accepted this revision. hokein added a comment. This revision is now accepted and ready to land.
Thanks, just a few nits. I think it is in a good shape, let's ship it! Please make sure the premerge-test is happy before landing the patch, the windows premerge test is failing now (https://buildkite.com/llvm-project/premerge-checks/builds/155660#0188764d-dc9f-4e8f-b489-c7b8f8b0c52a) ================ Comment at: clang-tools-extra/clang-tidy/misc/IncludeCleanerCheck.cpp:88 + SourceLocation Loc = D->getLocation(); + if (!SM->isWrittenInMainFile(SM->getSpellingLoc(Loc))) + continue; ---------------- VitaNuo wrote: > 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. > Is the expansion location of myfunc in the main file? If that's the case, we > need the expansion location indeed. Right. The expansion location here is a file location which points to the main-file. Would be nice if you add the above test into the unittest. ================ Comment at: clang-tools-extra/clang-tidy/misc/IncludeCleanerCheck.cpp:189-193 + std::optional<tooling::Replacement> Replacement = + HeaderIncludes.insert(llvm::StringRef{Spelling}.trim("\"<>"), Angled, + tooling::IncludeDirective::Include); + if (!Replacement.has_value()) + continue; ---------------- nit: we can simplify it like ``` if (auto Replacement = HeaderIncludes.insert(...)) diag(...); ``` ================ Comment at: clang-tools-extra/unittests/clang-tidy/IncludeCleanerTest.cpp:27 +namespace test { + +std::string ---------------- wrap all things into an anonymous namespace to avoid potential ODR violations. ================ Comment at: clang-tools-extra/unittests/clang-tidy/IncludeCleanerTest.cpp:70 + IgnoreHeaders += appendPathSystemIndependent({"baz", "qux"}); + IgnoreHeaders += ";vector"; + Opts.CheckOptions["IgnoreHeaders"] = llvm::StringRef{IgnoreHeaders}; ---------------- Instead of using 4 += statements, I think it is a bit clearer to use `llvm::formatv("bar.h;{1};{2};...", appendPathSystemIndependent(...), ....);` 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
