njames93 marked 2 inline comments as done. njames93 added a comment. 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, ================ Comment at: clang-tools-extra/clang-tidy/ClangTidyCheck.h:48 explicit MissingOptionError(std::string OptionName) - : OptionName(OptionName) {} + : OptionName(std::move(OptionName)) {} ---------------- aaron.ballman wrote: > It makes me sad how much we're having to sprinkle `std::move()` calls around > (I find the calls to `move()` to be more distracting that declaring the > parameter types to be `const std::string&`.) Were these found mechanically? These are me screwing around with my branches and will be removed ================ Comment at: clang-tools-extra/unittests/clang-tidy/LLVMModuleTest.cpp:224 } + +TEST(LLVMHeaderGuardCheckTest, FixHeaderGuardsWithComments) { ---------------- 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. 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
