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

[Frank Derry Wanye via Phabricator via llvm-commits]

ffrankies marked 3 inline comments as done.
ffrankies added a comment.

@aaron.ballman Can you please commit this on my behalf? My github username is ffrankies <https://github.com/ffrankies>.

And could you take a look at D72241 altera single work item barrier check <https://reviews.llvm.org/D72241>? It's also been updated and awaiting review.



================
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.");
----------------
aaron.ballman wrote:
> aaron.ballman wrote:
> > 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`
> The diagnostic here doesn't look quite right. This is the case where the source compiland is named poorly, but the diagnostic is talking about including files. It looks like there's test coverage missing for this.
My bad, I copied this line over the from other diagnostic and didn't change "including" to "compiling". Will update shortly


================
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"
----------------
aaron.ballman wrote:
> I assume it's also fine if the user does something really weird like: `#include "kernel.cl/foo.h"` ?
Yes, this is fine. The guide only specifies potential errors when the kernel source file is named kernel.cl, verilog.cl, or vhdl.cl. 

I've added additional test cases that use these names as directory names below.


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

https://reviews.llvm.org/D72218