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