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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
