[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:86
+  llvm::SmallVector<Decl *> MainFileDecls;
+  for (auto *D : Result.Nodes.getNodeAs<TranslationUnitDecl>("top")->decls()) {
+    SourceLocation Loc = D->getLocation();
----------------
PiotrZSL wrote:
> auto -> const Decl* 
`const` will not work, since the container I need to populate has to have `llvm::SmallVector<Decl *>` type due to the include-cleaner library interfaces.


================
Comment at: clang-tools-extra/clang-tidy/misc/IncludeCleanerCheck.cpp:88
+    SourceLocation Loc = D->getLocation();
+    if (!SM->isWrittenInMainFile(SM->getSpellingLoc(Loc)))
+      continue;
----------------
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.



================
Comment at: clang-tools-extra/clang-tidy/misc/IncludeCleanerCheck.cpp:81
+       Result.Nodes.getNodeAs<TranslationUnitDecl>("top")->decls()) {
+    if (!SM->isInMainFile(D->getLocation()))
+      continue;
----------------
kadircet wrote:
> 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)
> 
> also `SourceManager::isInMainFile` will follow `#line` directives, which can create confusion (a declaration from some other file will create a diagnostic in the main file), so let's use `isWrittenInMainFile` instead?
> 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)
> 
> also `SourceManager::isInMainFile` will follow `#line` directives, which can create confusion (a declaration from some other file will create a diagnostic in the main file), so let's use `isWrittenInMainFile` instead?




================
Comment at: clang-tools-extra/clang-tidy/misc/IncludeCleanerCheck.h:42
+    llvm::SmallVector<llvm::StringRef> SplitIgnoreHeaders;
+    llvm::StringRef{*IgnoreHeaders}.split(SplitIgnoreHeaders, ",");
+    for (const auto &Header : SplitIgnoreHeaders) {
----------------
PiotrZSL wrote:
> many check uses ; as separator, and uses parseStringList function to do that from OptionsUtils.
Thanks for the tip, I can use the OptionsUtils infrastructure indeed.


================
Comment at: clang-tools-extra/unittests/clang-tidy/IncludeCleanerTest.cpp:19
+#include "bar.h"
+#include <vector>
+#include "bar.h"
----------------
PiotrZSL wrote:
> make sure that those tests does not depend on actual system headers (check what headers are included here).
they don't, adding a declaration to the custom "vector.h" and then using it in the main file makes `vector` disappear from the list of unused includes.
On top of that, even if the test did somehow depend on the actual system headers, this wouldn't change anything in the test result, since the `vector` header would remain unused.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D148793