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

[Haojian Wu via Phabricator via cfe-commits]

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