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

Fri Jan 31 08:47:21 PST 2020

njames93 marked 2 inline comments as done.
njames93 added a comment.

In D73413#1851432 <https://reviews.llvm.org/D73413#1851432>, @tonyelewis wrote:

> Thanks so much for all the effort on this. I think it's really wonderful.
>
> I've added a couple of comments elsewhere.
>
> My other query: does/should this check cover types? Eg does/should it fire on a class definition in a .cpp file that isn't given internal-linkage? I'm inclined to say it should.

Support for checking types should be either opt-in(or opt-out) but never forced. I feel like it would generate too much noise.

================
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
----------------
tonyelewis wrote:
> njames93 wrote:
> > 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.
> FWIW, my instinct is that there are two separate questions:
> 
>  1. What sort of files should have their external-linkage definitions checked?
>  2. What sort of included files should be part of the search for adequate declarations that match those definitions?
> 
> ...and that my answers, in reverse order, are:
> 
>  2. All included files should be included (ie a check for a given external-linkage definition should be appeased by _any_ matching declaration in _any_ included file)
>  1. Only main files. And (to prevent lots of false-positives when the tool is run over headers), only "c", "cc", "cxx", "cpp" files by default but with an option to check in _all_ main files.
> 
> I think this has the merits that it:
>  * allows users to put their valid declaration in files with any extension they like
>  * defaults to only firing when run against c/cc/cxx/cpp files but provides the option to fire when run against a file with _any_ extension
> 
> 
> So I propose that there be one option and it be as follows:
> 
> 
> > .. option:: CheckExtDefnsInAllMainFiles
> > 
> > If set to `0` only check external-linkage definitions in main files with typical source-file extensions ("c", "cc", "cxx", "cpp").
> > If set to `1` check external linkage definitions in all main files.
> > Default value is `0`.
So current behaviour is kind of explained that you can either search in headers that are a starting substring of the source file or just search for all headers, I tried to explain it in the documentation. By default it only checks for definitions in files with the extension `c`, `cc`, `cpp` or `cxx`. I could add an option to check all definitions expanded in the main file, regardless of the extension.

================
Comment at: clang-tools-extra/docs/clang-tidy/checks/misc-missing-header-file-declaration.rst:8
+that don't have a declaration in the corresponding header file.
+
+A corresponding header is a header file whose name appears at the start of
----------------
tonyelewis wrote:
> I think it would be worth an extra comment here to explain to users why this situation is fishy and what they should do about it. Eg along the lines of:
> 
> > Giving a symbol external linkage indicates that you intend that symbol to be usable within other translation units, suggesting there should be a declaration of the symbol in a header. No such symbol was found in this translation unit.
> > 
> > If you want the symbol to be accessible from other translation units, add a declaration of it to a suitable header file included as part of this translation unit.
> > 
> > Otherwise, give the definition internal linkage, using `static` or an anonymous namespace.
> > 
> > If you think the symbol is already declared in a file that's included as part of this translation unit, check for any mismatches between the declaration and definition, including the namespace, spelling and any arguments.
I may copy that exact documentation. The current documentation is definitely needing some improvement.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D73413