tonyelewis added a comment.
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.
================
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:
> > 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`.
================
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
----------------
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.
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
