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

[Nathan James via Phabricator via cfe-commits]

njames93 added inline comments.


================
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:
> njames93 wrote:
> > gribozavr2 wrote:
> > > 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.
> > Would you say the best way would be check if the source file name starts with the header file name by default. Or is that very specific to Clang?
> > 
> > ```
> > /// <SomeHeaderImpl.cpp>
> > #include "SomeHeader.h"
> > ```
> > This file would check for declarations in `SomeHeader.h`
> > 
> > Or maybe check if the header file name starts with the source file?
> > 
> > ```
> > /// <SomeHeader.cpp>
> > #include "SomeHeaderImpl.h"
> > ```
> > This file would check for declarations in `SomeHeaderImpl.h`.
> > Or even check both?
> > Would you say the best way would be check if the source file name starts with the header file name by default. Or is that very specific to Clang?
> 
> I don't think it is too specific, it should cover many projects I think.
> 
> > Or maybe check if the header file name starts with the source file?
> 
> Seems like an unusual case to me, but I'm willing to be convinced otherwise.
I thought it was an unusual case and wasn't thinking it would be a good idea to add. I'll just set it up so that it will always look in header files whose names appear at the start of the main source file.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D73413