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 ---------------- 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. 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
