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