[PATCH] D73413: [clang-tidy] Add check to detect external definitions with no header declaration

[Dmitri Gribenko via Phabricator via cfe-commits]

gribozavr2 added inline comments.


================
Comment at: clang-tools-extra/clang-tidy/misc/MissingHeaderFileDeclarationCheck.cpp:29
+
+  bool isHeader() const {
+    return llvm::StringSwitch<bool>(Extension)
----------------
njames93 wrote:
> 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)?
> 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.

Does that really happen? That sounds like broken editor integration to me, because this is not how C++ without modules works...

> Would a better metric being matching on file extensions not corresponding to source files (c, cc, cxx, cpp)?

Yes, I think that would be better, if we have to sniff file extensions.


================
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
----------------
njames93 wrote:
> 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
That's the thing -- due to the lack of consistency in projects and style guides for C++, I think most projects will have to turn on CheckAnyHeader. We get implementation complexity in ClangTidy, false positives in high-profile projects, force users to configure ClangTidy to work well for their projects (unless we set CheckAnyHeader=1 by default... which then nobody would flip back to zero), and little benefit for end users.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D73413