[PATCH] D72218: [clang-tidy] new altera kernel name restriction check

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.


================
Comment at: clang-tools-extra/clang-tidy/altera/KernelNameRestrictionCheck.cpp:53
+void KernelNameRestrictionCheck::registerPPCallbacks(
+    const SourceManager &SM, Preprocessor *PP, Preprocessor *ModuleExpanderPP) {
+  PP->addPPCallbacks(
----------------
You can elide the identifier `ModuleExpanderPP` since it's not used in the call.


================
Comment at: clang-tools-extra/clang-tidy/altera/KernelNameRestrictionCheck.cpp:74-76
+    StringRef FileName = FilePath.substr(FilePath.find_last_of("/\\") + 1);
+    if (FileName.equals_lower("kernel.cl") ||
+        FileName.equals_lower("verilog.cl") || FileName.equals_lower("vhdl.cl"))
----------------
Rather than do path manipulations manually, I'd rather use `llvm::sys::path::filename()`


================
Comment at: clang-tools-extra/clang-tidy/altera/KernelNameRestrictionCheck.cpp:78
+      Check.diag(ID.Loc,
+                 "The imported kernel source file is named 'kernel.cl',"
+                 "'Verilog.cl', or 'VHDL.cl', which could cause compilation "
----------------
clang-tidy diagnostics are not meant to be grammatically correct, so I think this should be something more like: `including '%0' may cause additional compilation errors due to the name of the file; consider renaming the included file` and pass in the name of the file being included.


================
Comment at: clang-tools-extra/clang-tidy/altera/KernelNameRestrictionCheck.cpp:86-88
+  StringRef FileName = FilePath.substr(FilePath.find_last_of("/\\") + 1);
+  if (FileName.equals_lower("kernel.cl") ||
+      FileName.equals_lower("verilog.cl") || FileName.equals_lower("vhdl.cl"))
----------------
Similar here about using filesystem utilities.


================
Comment at: clang-tools-extra/clang-tidy/altera/KernelNameRestrictionCheck.cpp:90
+    Check.diag(SM.getLocForStartOfFile(SM.getMainFileID()),
+               "Naming your OpenCL kernel source file 'kernel.cl', 'Verilog.cl'"
+               ", or 'VHDL.cl' could cause compilation errors.");
----------------
Similar here, I would word it something like: `compiling a source file named '%0' may result in additional compilation errors due to the name of the file; consider renaming the source file`


================
Comment at: clang-tools-extra/test/clang-tidy/checkers/altera-kernel-name-restriction.cpp:40
+// The files can still have the forbidden names in them, so long as they're not the entire file name
+#include "some_kernel.cl"
+#include "other_Verilog.cl"
----------------
I assume it's also fine if the user does something really weird like: `#include "kernel.cl/foo.h"` ?


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

https://reviews.llvm.org/D72218