njames93 marked an inline comment as done.
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
----------------
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?
> > > > > >
> > > > > > ```
> > > > > > /// <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.
> Yep - I //think// I understand the current behaviour (but could well be wrong
> about that of course :) ).
>
> But IIUC, I share @gribozavr2's concerns that the current behaviour's default
> of only accepting matching declarations in included files //that match a
> specific pattern// is going to trigger a lot of false-positives in a lot of
> projects. And I don't think this check should care about //where// the
> matching declaration is found, so long as there is such a declaration in a
> file included within the TU.
>
> To my thinking, the implication of having a default policy that restricts the
> names of files in which we'll accept matching declarations is that we're
> policing the way that users name and organise their files (by spamming them
> with lots of false-positives if they don't do it in a way that satisfies our
> rule). I'd argue that file naming/organisation doesn't belong as part of this
> check (or even as part of any other clang-tidy check).
>
> The other half of my previous comment was about a
> `CheckExtDefnsInAllMainFiles` option but I think that that's less important
> and that having a restriction to `c`, `cc`, `cpp` and `cxx` seems pretty
> reasonable to me—it could always be broadened in the future.
>
> Overall, I think it's better to err on the side of //not// triggering the
> warning so as to avoid excessive false-positives; the check can always be
> made more sensitive in the future. I see the implications of that stance as
> being:
> * default to only checking the definitions in files that look like source
> files (so I think restricting to `c`, `cc`, `cpp` and `cxx` is good)
> * default to accepting matching declarations in all included files.
>
The reason for the unrelated header check is to prevent potential bugs. If a
declaration is in a completely unrelated header that happens to get included,
it more than likely could be a collision which could then break ODR rules
during linking. Perhaps a better thing to do would be if the decl isn't in a
"corresponding" header, emit a warning, but say there was a decl found in
`example.h`
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
