[PATCH] D83223: [clang-tidy] Header guard check can skip past license comment

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

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



================
Comment at: clang-tools-extra/clang-tidy/ClangTidyCheck.h:48
   explicit MissingOptionError(std::string OptionName)
-      : OptionName(OptionName) {}
+      : OptionName(std::move(OptionName)) {}
 
----------------
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?


================
Comment at: clang-tools-extra/clang-tidy/utils/HeaderGuard.cpp:313
         continue;
-
+      StringRef Opening = AddNewLine == NewLineInsertions::None ? "#ifndef "
+                          : AddNewLine == NewLineInsertions::One
----------------
njames93 wrote:
> clang-format(trunk) says what I submitted was correct, clang-format-10 is suggesting the change. Which do I trust??
Either is defensible, I'm fine with using what trunk says.


================
Comment at: clang-tools-extra/unittests/clang-tidy/LLVMModuleTest.cpp:12
 
 // FIXME: It seems this might be incompatible to dos path. Investigating.
 #if !defined(_WIN32)
----------------
Unrelated to this patch, but neat to know that there is zero testing of this on Windows.


================
Comment at: clang-tools-extra/unittests/clang-tidy/LLVMModuleTest.cpp:224
 }
+
+TEST(LLVMHeaderGuardCheckTest, FixHeaderGuardsWithComments) {
----------------
You seem to be missing tests that show the license and header guards are both correct and found no issues.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D83223/new/

https://reviews.llvm.org/D83223