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

[Aaron Ballman via Phabricator via llvm-commits]

aaron.ballman accepted this revision.
aaron.ballman added a comment.
This revision is now accepted and ready to land.

In D72218#2334787 <https://reviews.llvm.org/D72218#2334787>, @ffrankies wrote:

> Addressed comments by @aaron.ballman regarding the diagnostic warning.
>
> I tried to add a test case for when the filename is `kernel.cl`, `verilog.cl`, or `vhdl.cl`, but that did not work because the test suite appends `.tmp.cpp` to the end of the test files, and `kernel.cl.tmp.cpp` is not a restricted filename. If you know of a way to include this test case in the test suite, please let me know.

Oh, that's an interesting fact I hadn't considered before. I don't know of a better way to do that.

> In the meantime, I tested this functionality manually, and found a minor bug that has since been fixed.

Great!

> The bug was: if `kernel.cl` does not have any include directives, then the warning would not show up. Fixed this by rearranging the code to check the main file name before checking the include directives.

That makes sense to me.

The check LGTM aside from some minor nits. If you need me to commit on your behalf once you've made the changes, just let me know. Thanks!



================
Comment at: clang-tools-extra/clang-tidy/altera/KernelNameRestrictionCheck.cpp:60-63
+    SourceLocation HashLoc, const Token &IncludeTok, StringRef FileName,
+    bool IsAngled, CharSourceRange FilenameRange, const FileEntry *File,
+    StringRef SearchPath, StringRef RelativePath, const Module *Imported,
+    SrcMgr::CharacteristicKind FileType) {
----------------
It looks like you can leave a bunch of identifiers off the parameters to shorten the declaration and avoid unused parameter diagnostics in some compilers.


================
Comment at: clang-tools-extra/clang-tidy/altera/KernelNameRestrictionCheck.cpp:73-74
+  StringRef FileName = llvm::sys::path::filename(Entry->getName());
+  if (FileName.equals_lower("kernel.cl") ||
+      FileName.equals_lower("verilog.cl") || FileName.equals_lower("vhdl.cl"))
+    Check.diag(SM.getLocForStartOfFile(SM.getMainFileID()),
----------------
Rather than duplicate the checking logic here and below, I'd like a small helper function to check whether the name is problematic that gets called from both places.


================
Comment at: clang-tools-extra/docs/clang-tidy/checks/altera-kernel-name-restriction.rst:7
+Finds kernel files and include directives whose filename is `kernel.cl`,
+`Verilog.cl`, or `VHDL.cl`.
+
----------------
We should explicitly document that the check is case insensitive.


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

https://reviews.llvm.org/D72218