njames93 marked 2 inline comments as done.
njames93 added a comment.
I'll work up those other test cases
================
Comment at:
clang-tools-extra/clang-tidy/misc/MissingHeaderFileDeclarationCheck.cpp:29
+
+ bool isHeader() const {
+ return llvm::StringSwitch<bool>(Extension)
----------------
gribozavr2 wrote:
> I think we should consider any file other than the main file to be a header
> (anything brought in by `#include`). Projects have lots of different
> convention -- for example, LLVM uses a `.inc` extension for some generated
> files and x-macro files. The standard library does not have any extensions at
> all (and I'm sure some projects imitate that) etc.
I agree in part, but there is one case that I didn't want to happen and that is
when clang-tidy is ran as a text editor plugin. Often it will blindly treat the
current open file as the main file which would lead to a lot of false
positives, Would a better metric being matching on file extensions not
corresponding to source files (c, cc, cxx, cpp)?
================
Comment at:
clang-tools-extra/clang-tidy/misc/MissingHeaderFileDeclarationCheck.cpp:75
+ continue;
+ if (AnyHeader || File->NamePart.equals_lower(ThisFile->NamePart))
+ return; // Found a good candidate for matching decl
----------------
gribozavr2 wrote:
> This heuristic should be a lot more complex. In practice people have more
> complex naming conventions (for example, in Clang, Sema.h corresponds to many
> files named SemaSomethingSomething.cpp).
>
> I think there is a high chance that checking only a header with a matching
> name will satisfy too few projects to be worth implementing.
>
> On the other hand, if you could use C++ or Clang modules to narrow down the
> list of headers where the declaration should appear, that would be a much
> stronger signal.
That is the reason I added the CheckAnyHeader option. For small projects the
matching name would be usually be enough, but for big complex projects there is
no feasible check that would work. Falling back to making sure every external
definition has a declaration in at least one header will always be a good
warning
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D73413/new/
https://reviews.llvm.org/D73413
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
