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

[Haojian Wu via Phabricator via cfe-commits]

hokein added a comment.

The only thing is the source location, see my comments for details, otherwise looks good in general.



================
Comment at: clang-tools-extra/clang-tidy/misc/IncludeCleanerCheck.cpp:116
+  }
+  walkUsed(MainFileDecls, RecordedPreprocessor.MacroReferences, &RecordedPI,
+           *SM,
----------------
it is sad that we duplicate the include-cleaner analyse implementation (the only difference is that here we record the references for missing-includes), I think we should find a way to share the code in the future.

No action required in this patch, can you add a FIXME? 


================
Comment at: clang-tools-extra/clang-tidy/misc/IncludeCleanerCheck.cpp:194
+
+    diag(Inc.SymRefLocation, "no header providing %0 is directly included")
+        << Spelling
----------------
I think we should use the spelling location -- `Inc.SymRefLocation` can be a macro location which can points to another file, we intend to show diagnostics for main-file only, and walkUsed has some internal logic which guarantees that the spelling loc of returned refs is main-file.


================
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:
> > 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.


================
Comment at: clang-tools-extra/clang-tidy/misc/IncludeCleanerCheck.h:21
+#include "clang/Lex/HeaderSearch.h"
+#include "clang/Tooling/Syntax/Tokens.h"
+#include "llvm/Support/Regex.h"
----------------
can you cleanup the includes here? looks like there are some unused-includes now, at least `Syntax/Tokens.h` from the first glance.


================
Comment at: clang-tools-extra/docs/clang-tidy/checks/misc/include-cleaner.rst:4
+misc-include-cleaner
+======================
+
----------------
nit: remove extra trailing `==`


================
Comment at: clang-tools-extra/unittests/clang-tidy/IncludeCleanerTest.cpp:1
+//===--- IncludeCleanerCheck.cpp - clang-tidy -----------------------------===//
+//
----------------
nit: IncludeCleanerTest.cpp


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D148793