[PATCH] D73413: [clang-tidy] Add check to detect external definitions with no header declaration
tonyelewis added a comment.
Thank you very much for coming back to this and for bringing the changes up to
date. I still think this would be really valuable.
> Right now it will flag template definitions in source files, Is this good or
> bad behaviour?
Do you mean that both of the following would be flagged?
// a.cpp
template void f() {}
template class C {};
I'm not sure how I feel about that. It feels to me like a less clear signal
that the implementation file is mistakenly out of sync with the headers. Also,
I think it's better to err on the side of being conservative about what we flag
at first, knowing we can always consider flagging more later on. But I'm not
well enough informed to have strong feelings.
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
[PATCH] D73413: [clang-tidy] Add check to detect external definitions with no header declaration
tonyelewis added a comment.
> Support for checking types should be either opt-in(or opt-out) but never
> forced. I feel like it would generate too much noise.
Okey dokes. That option could always be added as another feature in the future.
Thanks very much for all work on this.
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:
> 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?
> > > > >
> > > > > ```
> > > > > ///
> > > > > #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?
> > > > >
> > > > > ```
> > > > > ///
> > > > > #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 def
[PATCH] D73413: [clang-tidy] Add check to detect external definitions with no header declaration
tonyelewis 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 njames93 wrote: > tonyelewis wrote: > > njames93 wrote: > > > 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? > > > > > > > > > > > > > > ``` > > > > > > > /// > > > > > > > #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? > > > > > > > > > > > > > > ``` > > > > > > > /// > > > > > > > #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:: CheckExtDefnsInA
[PATCH] D73413: [clang-tidy] Add check to detect external definitions with no header declaration
tonyelewis added a comment. Thanks very much. 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
[PATCH] D73413: [clang-tidy] Add check to detect external definitions with no header declaration
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?
> > >
> > > ```
> > > ///
> > > #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?
> > >
> > > ```
> > > ///
> > > #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 u
[PATCH] D73413: [clang-tidy] Add check to detect external definitions with no header declaration
tonyelewis added a comment. @njames93 Thank you very much for your continued input on this. Is your ping for an automated process or a human? If the latter, is it for me or for the reviewers? 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
