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