aaron.ballman added a comment. In D83223#2147247 <https://reviews.llvm.org/D83223#2147247>, @njames93 wrote:
> In D83223#2147072 <https://reviews.llvm.org/D83223#2147072>, @aaron.ballman > wrote: > > > > // This is not identified as a license comment as the > > > // block is followed by code. > > > void foo(); > > > > FWIW: > > https://github.com/GrammaTech/gtirb-pprinter/blob/master/include/gtirb_pprinter/AttPrettyPrinter.hpp > > or > > https://github.com/GrammaTech/gtirb/blob/master/include/gtirb/AuxData.hpp > > (so there are projects which do not put a newline between the license and > > code). > > > Short of creating an AI that understands context it won't be possible to > determine the difference between license and general documentation, in any > case I feel this heuristic is the safest way to ensure good coverage with > minimised risk of inserting the guard in the middle of documentation, My instinct is that we shouldn't be trying to play those games in the first place and should consider *all* leading comments and empty (whitespace-only) lines as part of the "license" and expect the first significant token to be the header guard. e.g., this isn't about the license at all, it's about whether you can have prose before the header guard or not. It's not uncommon for projects to put prose before header guards, nor is it uncommon for it to go after the header guards. tbh, that feels a bit like an option for the feature rather than an automatic behavior because I could also see a project wanting to enforce a consistent style. ================ Comment at: clang-tools-extra/unittests/clang-tidy/LLVMModuleTest.cpp:224 } + +TEST(LLVMHeaderGuardCheckTest, FixHeaderGuardsWithComments) { ---------------- njames93 wrote: > aaron.ballman wrote: > > You seem to be missing tests that show the license and header guards are > > both correct and found no issues. > That wouldn't be testing new behaviour added here. The check can already find > header guards when there is a license, The code I have added only affects > when no guard is found and it needs to add one. I can still add those cases > for piece of mind. I'd appreciate the test coverage because it's not always immediately clear why some of the new tests are expected to fail. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D83223/new/ https://reviews.llvm.org/D83223 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
