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

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

In D72218#2381149 , @ffrankies wrote:

> Moved test files `KERNEL.cl`, `VHDL.cl` and `vERILOG.cl` to the `uppercase` 
> subdirectory to prevent filename clashes in some environments.
>
> This is in response to the buildbot failure where `Verilog.cl`, `KERNEL.cl`, 
> and `VHDL.cl` were not present in the buildbot output despite showing up as 
> present in Differential.
>
> @aaron.ballman Can you please try committing this again?

I've recommit in 9ca6fc4e095f9aacd70e406e640472ad2d370553 
, thanks!


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

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

[Frank Derry Wanye via Phabricator via cfe-commits]

ffrankies updated this revision to Diff 303680.
ffrankies added a comment.

Moved test files `KERNEL.cl`, `VHDL.cl` and `vERILOG.cl` to the `uppercase` 
subdirectory to prevent filename clashes in some environments.

This is in response to the buildbot failure where `Verilog.cl`, `KERNEL.cl`, 
and `VHDL.cl` were not present in the buildbot output despite being showing up 
as present in Differential.

@aaron.ballman Can you please try committing this again?


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

https://reviews.llvm.org/D72218

Files:
  clang-tools-extra/clang-tidy/altera/AlteraTidyModule.cpp
  clang-tools-extra/clang-tidy/altera/CMakeLists.txt
  clang-tools-extra/clang-tidy/altera/KernelNameRestrictionCheck.cpp
  clang-tools-extra/clang-tidy/altera/KernelNameRestrictionCheck.h
  clang-tools-extra/docs/ReleaseNotes.rst
  clang-tools-extra/docs/clang-tidy/checks/altera-kernel-name-restriction.rst
  clang-tools-extra/docs/clang-tidy/checks/list.rst
  
clang-tools-extra/test/clang-tidy/checkers/Inputs/altera-kernel-name-restriction/Verilog.cl
  
clang-tools-extra/test/clang-tidy/checkers/Inputs/altera-kernel-name-restriction/kernel.cl
  
clang-tools-extra/test/clang-tidy/checkers/Inputs/altera-kernel-name-restriction/kernel.h
  
clang-tools-extra/test/clang-tidy/checkers/Inputs/altera-kernel-name-restriction/other_Verilog.cl
  
clang-tools-extra/test/clang-tidy/checkers/Inputs/altera-kernel-name-restriction/otherdir/vhdl.cl
  
clang-tools-extra/test/clang-tidy/checkers/Inputs/altera-kernel-name-restriction/otherthing.cl
  
clang-tools-extra/test/clang-tidy/checkers/Inputs/altera-kernel-name-restriction/some/dir/kernel.cl
  
clang-tools-extra/test/clang-tidy/checkers/Inputs/altera-kernel-name-restriction/some/kernel.cl/foo.h
  
clang-tools-extra/test/clang-tidy/checkers/Inputs/altera-kernel-name-restriction/some/verilog.cl/foo.h
  
clang-tools-extra/test/clang-tidy/checkers/Inputs/altera-kernel-name-restriction/some/vhdl.cl/foo.h
  
clang-tools-extra/test/clang-tidy/checkers/Inputs/altera-kernel-name-restriction/some_kernel.cl
  
clang-tools-extra/test/clang-tidy/checkers/Inputs/altera-kernel-name-restriction/somedir/verilog.cl
  
clang-tools-extra/test/clang-tidy/checkers/Inputs/altera-kernel-name-restriction/thing.h
  
clang-tools-extra/test/clang-tidy/checkers/Inputs/altera-kernel-name-restriction/uppercase/KERNEL.cl
  
clang-tools-extra/test/clang-tidy/checkers/Inputs/altera-kernel-name-restriction/uppercase/VHDL.cl
  
clang-tools-extra/test/clang-tidy/checkers/Inputs/altera-kernel-name-restriction/uppercase/vERILOG.cl
  
clang-tools-extra/test/clang-tidy/checkers/Inputs/altera-kernel-name-restriction/verilog.h
  
clang-tools-extra/test/clang-tidy/checkers/Inputs/altera-kernel-name-restriction/vhdl.CL
  
clang-tools-extra/test/clang-tidy/checkers/Inputs/altera-kernel-name-restriction/vhdl.h
  
clang-tools-extra/test/clang-tidy/checkers/Inputs/altera-kernel-name-restriction/vhdl_number_two.cl
  clang-tools-extra/test/clang-tidy/checkers/altera-kernel-name-restriction.cpp
  llvm/utils/gn/secondary/clang-tools-extra/clang-tidy/altera/BUILD.gn

Index: llvm/utils/gn/secondary/clang-tools-extra/clang-tidy/altera/BUILD.gn
===
--- llvm/utils/gn/secondary/clang-tools-extra/clang-tidy/altera/BUILD.gn
+++ llvm/utils/gn/secondary/clang-tools-extra/clang-tidy/altera/BUILD.gn
@@ -13,6 +13,7 @@
   ]
   sources = [
 "AlteraTidyModule.cpp",
+"KernelNameRestrictionCheck.cpp",
 "StructPackAlignCheck.cpp",
   ]
 }
Index: clang-tools-extra/test/clang-tidy/checkers/altera-kernel-name-restriction.cpp
===
--- /dev/null
+++ clang-tools-extra/test/clang-tidy/checkers/altera-kernel-name-restriction.cpp
@@ -0,0 +1,55 @@
+// RUN: %check_clang_tidy %s altera-kernel-name-restriction %t -- -- -I%S/Inputs/altera-kernel-name-restriction
+// RUN: %check_clang_tidy -check-suffix=UPPERCASE %s altera-kernel-name-restriction %t -- -- -I%S/Inputs/altera-kernel-name-restriction/uppercase -DUPPERCASE
+
+#ifdef UPPERCASE
+// The warning should be triggered regardless of capitalization
+#include "KERNEL.cl"
+// CHECK-MESSAGES-UPPERCASE: :[[@LINE-1]]:1: warning: including 'KERNEL.cl' may cause additional compilation errors due to the name of the kernel source file; consider renaming the included kernel source file [altera-kernel-name-restriction]
+#include "vERILOG.cl"
+// CHECK-MESSAGES-UPPERCASE: :[[@LINE-1]]:1: warning: including 'vERILOG.cl' may cause additional compilation errors due to the name of the kernel source file; consider renaming the included kernel source file [altera-kernel-name-restriction]
+#include "VHDL.cl"
+// CHECK-MESSAGES-UPPERCASE: :[[@LINE-1]]:1: warning: including 'VHDL.cl' may cause additional compilation errors due to the name of the kernel source file; consider renaming the included kernel source file [altera-kernel-name-restriction]
+#else 
+// These 

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

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman closed this revision.
aaron.ballman added a comment.

In D72218#2364297 , @ffrankies wrote:

> @aaron.ballman Can you please commit this on my behalf? My github username is 
> ffrankies .

I've commit on your behalf in 43a38a65233039b5e71797a644d41a890f8d7f2b 
, thank 
you!

> And could you take a look at D72241 altera single work item barrier check 
> ? It's also been updated and awaiting review.

I'll take a look, thank you for pinging it.


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

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

[Frank Derry Wanye via Phabricator via cfe-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 .

And could you take a look at D72241 altera single work item barrier check 
? 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

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Frank Derry Wanye via Phabricator via cfe-commits]

ffrankies updated this revision to Diff 301865.
ffrankies marked an inline comment as done.
ffrankies added a comment.

Implemented changes requested by @aaron.ballman

- Added a helper function that implements the string comparison logic
- Clarified that check is case insensitive
- Removed unused identifiers from 
`KernelNameRestrictionPPCallbacks::InclusionDirective` function


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

https://reviews.llvm.org/D72218

Files:
  clang-tools-extra/clang-tidy/altera/AlteraTidyModule.cpp
  clang-tools-extra/clang-tidy/altera/CMakeLists.txt
  clang-tools-extra/clang-tidy/altera/KernelNameRestrictionCheck.cpp
  clang-tools-extra/clang-tidy/altera/KernelNameRestrictionCheck.h
  clang-tools-extra/docs/ReleaseNotes.rst
  clang-tools-extra/docs/clang-tidy/checks/altera-kernel-name-restriction.rst
  clang-tools-extra/docs/clang-tidy/checks/list.rst
  
clang-tools-extra/test/clang-tidy/checkers/Inputs/altera-kernel-name-restriction/KERNEL.cl
  
clang-tools-extra/test/clang-tidy/checkers/Inputs/altera-kernel-name-restriction/VHDL.cl
  
clang-tools-extra/test/clang-tidy/checkers/Inputs/altera-kernel-name-restriction/Verilog.cl
  
clang-tools-extra/test/clang-tidy/checkers/Inputs/altera-kernel-name-restriction/kernel.cl
  
clang-tools-extra/test/clang-tidy/checkers/Inputs/altera-kernel-name-restriction/kernel.h
  
clang-tools-extra/test/clang-tidy/checkers/Inputs/altera-kernel-name-restriction/other_Verilog.cl
  
clang-tools-extra/test/clang-tidy/checkers/Inputs/altera-kernel-name-restriction/otherdir/vhdl.cl
  
clang-tools-extra/test/clang-tidy/checkers/Inputs/altera-kernel-name-restriction/otherthing.cl
  
clang-tools-extra/test/clang-tidy/checkers/Inputs/altera-kernel-name-restriction/some/dir/kernel.cl
  
clang-tools-extra/test/clang-tidy/checkers/Inputs/altera-kernel-name-restriction/some/kernel.cl/foo.h
  
clang-tools-extra/test/clang-tidy/checkers/Inputs/altera-kernel-name-restriction/some/verilog.cl/foo.h
  
clang-tools-extra/test/clang-tidy/checkers/Inputs/altera-kernel-name-restriction/some/vhdl.cl/foo.h
  
clang-tools-extra/test/clang-tidy/checkers/Inputs/altera-kernel-name-restriction/some_kernel.cl
  
clang-tools-extra/test/clang-tidy/checkers/Inputs/altera-kernel-name-restriction/somedir/verilog.cl
  
clang-tools-extra/test/clang-tidy/checkers/Inputs/altera-kernel-name-restriction/thing.h
  
clang-tools-extra/test/clang-tidy/checkers/Inputs/altera-kernel-name-restriction/vERILOG.cl
  
clang-tools-extra/test/clang-tidy/checkers/Inputs/altera-kernel-name-restriction/verilog.h
  
clang-tools-extra/test/clang-tidy/checkers/Inputs/altera-kernel-name-restriction/vhdl.CL
  
clang-tools-extra/test/clang-tidy/checkers/Inputs/altera-kernel-name-restriction/vhdl.h
  
clang-tools-extra/test/clang-tidy/checkers/Inputs/altera-kernel-name-restriction/vhdl_number_two.cl
  clang-tools-extra/test/clang-tidy/checkers/altera-kernel-name-restriction.cpp
  llvm/utils/gn/secondary/clang-tools-extra/clang-tidy/altera/BUILD.gn

Index: llvm/utils/gn/secondary/clang-tools-extra/clang-tidy/altera/BUILD.gn
===
--- llvm/utils/gn/secondary/clang-tools-extra/clang-tidy/altera/BUILD.gn
+++ llvm/utils/gn/secondary/clang-tools-extra/clang-tidy/altera/BUILD.gn
@@ -13,6 +13,7 @@
   ]
   sources = [
 "AlteraTidyModule.cpp",
+"KernelNameRestrictionCheck.cpp",
 "StructPackAlignCheck.cpp",
   ]
 }
Index: clang-tools-extra/test/clang-tidy/checkers/altera-kernel-name-restriction.cpp
===
--- /dev/null
+++ clang-tools-extra/test/clang-tidy/checkers/altera-kernel-name-restriction.cpp
@@ -0,0 +1,50 @@
+// RUN: %check_clang_tidy %s altera-kernel-name-restriction %t -- -- -I%S/Inputs/altera-kernel-name-restriction
+
+// These are the banned kernel filenames, and should trigger warnings
+#include "kernel.cl"
+// CHECK-MESSAGES: :[[@LINE-1]]:1: warning: including 'kernel.cl' may cause additional compilation errors due to the name of the kernel source file; consider renaming the included kernel source file [altera-kernel-name-restriction]
+#include "Verilog.cl"
+// CHECK-MESSAGES: :[[@LINE-1]]:1: warning: including 'Verilog.cl' may cause additional compilation errors due to the name of the kernel source file; consider renaming the included kernel source file [altera-kernel-name-restriction]
+#include "VHDL.cl"
+// CHECK-MESSAGES: :[[@LINE-1]]:1: warning: including 'VHDL.cl' may cause additional compilation errors due to the name of the kernel source file; consider renaming the included kernel source file [altera-kernel-name-restriction]
+
+// The warning should be triggered regardless of capitalization
+#include "KERNEL.cl"
+// CHECK-MESSAGES: :[[@LINE-1]]:1: warning: including 'KERNEL.cl' may cause additional compilation errors due to the name of the kernel source file; consider renaming the included kernel source file [altera-kernel-name-restriction]
+#include 

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

[Aaron Ballman via Phabricator via cfe-commits]

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

In 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 , 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

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Frank Derry Wanye via Phabricator via cfe-commits]

ffrankies updated this revision to Diff 298601.
ffrankies marked 7 inline comments as done.
ffrankies added a comment.

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. In the meantime, I tested this functionality manually, and 
found a minor bug that has since been fixed.

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.


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

https://reviews.llvm.org/D72218

Files:
  clang-tools-extra/clang-tidy/altera/AlteraTidyModule.cpp
  clang-tools-extra/clang-tidy/altera/CMakeLists.txt
  clang-tools-extra/clang-tidy/altera/KernelNameRestrictionCheck.cpp
  clang-tools-extra/clang-tidy/altera/KernelNameRestrictionCheck.h
  clang-tools-extra/docs/ReleaseNotes.rst
  clang-tools-extra/docs/clang-tidy/checks/altera-kernel-name-restriction.rst
  clang-tools-extra/docs/clang-tidy/checks/list.rst
  
clang-tools-extra/test/clang-tidy/checkers/Inputs/altera-kernel-name-restriction/KERNEL.cl
  
clang-tools-extra/test/clang-tidy/checkers/Inputs/altera-kernel-name-restriction/VHDL.cl
  
clang-tools-extra/test/clang-tidy/checkers/Inputs/altera-kernel-name-restriction/Verilog.cl
  
clang-tools-extra/test/clang-tidy/checkers/Inputs/altera-kernel-name-restriction/kernel.cl
  
clang-tools-extra/test/clang-tidy/checkers/Inputs/altera-kernel-name-restriction/kernel.h
  
clang-tools-extra/test/clang-tidy/checkers/Inputs/altera-kernel-name-restriction/other_Verilog.cl
  
clang-tools-extra/test/clang-tidy/checkers/Inputs/altera-kernel-name-restriction/otherdir/vhdl.cl
  
clang-tools-extra/test/clang-tidy/checkers/Inputs/altera-kernel-name-restriction/otherthing.cl
  
clang-tools-extra/test/clang-tidy/checkers/Inputs/altera-kernel-name-restriction/some/dir/kernel.cl
  
clang-tools-extra/test/clang-tidy/checkers/Inputs/altera-kernel-name-restriction/some/kernel.cl/foo.h
  
clang-tools-extra/test/clang-tidy/checkers/Inputs/altera-kernel-name-restriction/some/verilog.cl/foo.h
  
clang-tools-extra/test/clang-tidy/checkers/Inputs/altera-kernel-name-restriction/some/vhdl.cl/foo.h
  
clang-tools-extra/test/clang-tidy/checkers/Inputs/altera-kernel-name-restriction/some_kernel.cl
  
clang-tools-extra/test/clang-tidy/checkers/Inputs/altera-kernel-name-restriction/somedir/verilog.cl
  
clang-tools-extra/test/clang-tidy/checkers/Inputs/altera-kernel-name-restriction/thing.h
  
clang-tools-extra/test/clang-tidy/checkers/Inputs/altera-kernel-name-restriction/vERILOG.cl
  
clang-tools-extra/test/clang-tidy/checkers/Inputs/altera-kernel-name-restriction/verilog.h
  
clang-tools-extra/test/clang-tidy/checkers/Inputs/altera-kernel-name-restriction/vhdl.CL
  
clang-tools-extra/test/clang-tidy/checkers/Inputs/altera-kernel-name-restriction/vhdl.h
  
clang-tools-extra/test/clang-tidy/checkers/Inputs/altera-kernel-name-restriction/vhdl_number_two.cl
  clang-tools-extra/test/clang-tidy/checkers/altera-kernel-name-restriction.cpp
  llvm/utils/gn/secondary/clang-tools-extra/clang-tidy/altera/BUILD.gn

Index: llvm/utils/gn/secondary/clang-tools-extra/clang-tidy/altera/BUILD.gn
===
--- llvm/utils/gn/secondary/clang-tools-extra/clang-tidy/altera/BUILD.gn
+++ llvm/utils/gn/secondary/clang-tools-extra/clang-tidy/altera/BUILD.gn
@@ -13,6 +13,7 @@
   ]
   sources = [
 "AlteraTidyModule.cpp",
+"KernelNameRestrictionCheck.cpp",
 "StructPackAlignCheck.cpp",
   ]
 }
Index: clang-tools-extra/test/clang-tidy/checkers/altera-kernel-name-restriction.cpp
===
--- /dev/null
+++ clang-tools-extra/test/clang-tidy/checkers/altera-kernel-name-restriction.cpp
@@ -0,0 +1,50 @@
+// RUN: %check_clang_tidy %s altera-kernel-name-restriction %t -- -- -I%S/Inputs/altera-kernel-name-restriction
+
+// These are the banned kernel filenames, and should trigger warnings
+#include "kernel.cl"
+// CHECK-MESSAGES: :[[@LINE-1]]:1: warning: including 'kernel.cl' may cause additional compilation errors due to the name of the kernel source file; consider renaming the included kernel source file [altera-kernel-name-restriction]
+#include "Verilog.cl"
+// CHECK-MESSAGES: :[[@LINE-1]]:1: warning: including 'Verilog.cl' may cause additional compilation errors due to the name of the kernel source file; consider renaming the included kernel source file [altera-kernel-name-restriction]
+#include "VHDL.cl"
+// CHECK-MESSAGES: :[[@LINE-1]]:1: warning: including 'VHDL.cl' may cause additional compilation errors 

[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: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:
> 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.


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

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

[Frank Derry Wanye via Phabricator via cfe-commits]

ffrankies updated this revision to Diff 295697.
ffrankies removed a project: LLVM.
ffrankies added a comment.
Herald added a project: LLVM.

Addressed changes requested by @Eugene.Zelenko and @aaron.ballman


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

https://reviews.llvm.org/D72218

Files:
  clang-tools-extra/clang-tidy/altera/AlteraTidyModule.cpp
  clang-tools-extra/clang-tidy/altera/CMakeLists.txt
  clang-tools-extra/clang-tidy/altera/KernelNameRestrictionCheck.cpp
  clang-tools-extra/clang-tidy/altera/KernelNameRestrictionCheck.h
  clang-tools-extra/docs/ReleaseNotes.rst
  clang-tools-extra/docs/clang-tidy/checks/altera-kernel-name-restriction.rst
  clang-tools-extra/docs/clang-tidy/checks/list.rst
  
clang-tools-extra/test/clang-tidy/checkers/Inputs/altera-kernel-name-restriction/KERNEL.cl
  
clang-tools-extra/test/clang-tidy/checkers/Inputs/altera-kernel-name-restriction/VHDL.cl
  
clang-tools-extra/test/clang-tidy/checkers/Inputs/altera-kernel-name-restriction/Verilog.cl
  
clang-tools-extra/test/clang-tidy/checkers/Inputs/altera-kernel-name-restriction/kernel.cl
  
clang-tools-extra/test/clang-tidy/checkers/Inputs/altera-kernel-name-restriction/kernel.h
  
clang-tools-extra/test/clang-tidy/checkers/Inputs/altera-kernel-name-restriction/other_Verilog.cl
  
clang-tools-extra/test/clang-tidy/checkers/Inputs/altera-kernel-name-restriction/otherdir/vhdl.cl
  
clang-tools-extra/test/clang-tidy/checkers/Inputs/altera-kernel-name-restriction/otherthing.cl
  
clang-tools-extra/test/clang-tidy/checkers/Inputs/altera-kernel-name-restriction/some/dir/kernel.cl
  
clang-tools-extra/test/clang-tidy/checkers/Inputs/altera-kernel-name-restriction/some/kernel.cl/foo.h
  
clang-tools-extra/test/clang-tidy/checkers/Inputs/altera-kernel-name-restriction/some/verilog.cl/foo.h
  
clang-tools-extra/test/clang-tidy/checkers/Inputs/altera-kernel-name-restriction/some/vhdl.cl/foo.h
  
clang-tools-extra/test/clang-tidy/checkers/Inputs/altera-kernel-name-restriction/some_kernel.cl
  
clang-tools-extra/test/clang-tidy/checkers/Inputs/altera-kernel-name-restriction/somedir/verilog.cl
  
clang-tools-extra/test/clang-tidy/checkers/Inputs/altera-kernel-name-restriction/thing.h
  
clang-tools-extra/test/clang-tidy/checkers/Inputs/altera-kernel-name-restriction/vERILOG.cl
  
clang-tools-extra/test/clang-tidy/checkers/Inputs/altera-kernel-name-restriction/verilog.h
  
clang-tools-extra/test/clang-tidy/checkers/Inputs/altera-kernel-name-restriction/vhdl.CL
  
clang-tools-extra/test/clang-tidy/checkers/Inputs/altera-kernel-name-restriction/vhdl.h
  
clang-tools-extra/test/clang-tidy/checkers/Inputs/altera-kernel-name-restriction/vhdl_number_two.cl
  clang-tools-extra/test/clang-tidy/checkers/altera-kernel-name-restriction.cpp
  llvm/utils/gn/secondary/clang-tools-extra/clang-tidy/altera/BUILD.gn

Index: llvm/utils/gn/secondary/clang-tools-extra/clang-tidy/altera/BUILD.gn
===
--- llvm/utils/gn/secondary/clang-tools-extra/clang-tidy/altera/BUILD.gn
+++ llvm/utils/gn/secondary/clang-tools-extra/clang-tidy/altera/BUILD.gn
@@ -13,6 +13,7 @@
   ]
   sources = [
 "AlteraTidyModule.cpp",
+"KernelNameRestrictionCheck.cpp",
 "StructPackAlignCheck.cpp",
   ]
 }
Index: clang-tools-extra/test/clang-tidy/checkers/altera-kernel-name-restriction.cpp
===
--- /dev/null
+++ clang-tools-extra/test/clang-tidy/checkers/altera-kernel-name-restriction.cpp
@@ -0,0 +1,50 @@
+// RUN: %check_clang_tidy %s altera-kernel-name-restriction %t -- -- -I%S/Inputs/altera-kernel-name-restriction
+
+// These are the banned kernel filenames, and should trigger warnings
+#include "kernel.cl"
+// CHECK-MESSAGES: :[[@LINE-1]]:1: warning: including 'kernel.cl' may cause additional compilation errors due to the name of the kernel source file; consider renaming the included kernel source file [altera-kernel-name-restriction]
+#include "Verilog.cl"
+// CHECK-MESSAGES: :[[@LINE-1]]:1: warning: including 'Verilog.cl' may cause additional compilation errors due to the name of the kernel source file; consider renaming the included kernel source file [altera-kernel-name-restriction]
+#include "VHDL.cl"
+// CHECK-MESSAGES: :[[@LINE-1]]:1: warning: including 'VHDL.cl' may cause additional compilation errors due to the name of the kernel source file; consider renaming the included kernel source file [altera-kernel-name-restriction]
+
+// The warning should be triggered regardless of capitalization
+#include "KERNEL.cl"
+// CHECK-MESSAGES: :[[@LINE-1]]:1: warning: including 'KERNEL.cl' may cause additional compilation errors due to the name of the kernel source file; consider renaming the included kernel source file [altera-kernel-name-restriction]
+#include "vERILOG.cl"
+// CHECK-MESSAGES: :[[@LINE-1]]:1: warning: including 'vERILOG.cl' may cause additional compilation errors due to the name of the kernel source file; consider 

[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 , 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

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

[Eugene Zelenko via Phabricator via cfe-commits]

Eugene.Zelenko added inline comments.



Comment at: 
clang-tools-extra/docs/clang-tidy/checks/altera-kernel-name-restriction.rst:4
+altera-kernel-name-restriction
+
+

Please make same length as text above.


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

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

[Frank Derry Wanye via Phabricator via cfe-commits]

ffrankies updated this revision to Diff 294215.
ffrankies edited the summary of this revision.
ffrankies added a comment.
Herald added a project: LLVM.
Herald added a subscriber: llvm-commits.

- Rebased code and fixed merge conflicts with  D66564 

- Added KernelNameRestrictionCheck.cpp to BUILD.gn


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

https://reviews.llvm.org/D72218

Files:
  clang-tools-extra/clang-tidy/altera/AlteraTidyModule.cpp
  clang-tools-extra/clang-tidy/altera/CMakeLists.txt
  clang-tools-extra/clang-tidy/altera/KernelNameRestrictionCheck.cpp
  clang-tools-extra/clang-tidy/altera/KernelNameRestrictionCheck.h
  clang-tools-extra/docs/ReleaseNotes.rst
  clang-tools-extra/docs/clang-tidy/checks/altera-kernel-name-restriction.rst
  clang-tools-extra/docs/clang-tidy/checks/list.rst
  
clang-tools-extra/test/clang-tidy/checkers/Inputs/altera-kernel-name-restriction/KERNEL.cl
  
clang-tools-extra/test/clang-tidy/checkers/Inputs/altera-kernel-name-restriction/VHDL.cl
  
clang-tools-extra/test/clang-tidy/checkers/Inputs/altera-kernel-name-restriction/Verilog.cl
  
clang-tools-extra/test/clang-tidy/checkers/Inputs/altera-kernel-name-restriction/kernel.cl
  
clang-tools-extra/test/clang-tidy/checkers/Inputs/altera-kernel-name-restriction/kernel.h
  
clang-tools-extra/test/clang-tidy/checkers/Inputs/altera-kernel-name-restriction/other_Verilog.cl
  
clang-tools-extra/test/clang-tidy/checkers/Inputs/altera-kernel-name-restriction/otherdir/vhdl.cl
  
clang-tools-extra/test/clang-tidy/checkers/Inputs/altera-kernel-name-restriction/otherthing.cl
  
clang-tools-extra/test/clang-tidy/checkers/Inputs/altera-kernel-name-restriction/some/dir/kernel.cl
  
clang-tools-extra/test/clang-tidy/checkers/Inputs/altera-kernel-name-restriction/some_kernel.cl
  
clang-tools-extra/test/clang-tidy/checkers/Inputs/altera-kernel-name-restriction/somedir/verilog.cl
  
clang-tools-extra/test/clang-tidy/checkers/Inputs/altera-kernel-name-restriction/thing.h
  
clang-tools-extra/test/clang-tidy/checkers/Inputs/altera-kernel-name-restriction/vERILOG.cl
  
clang-tools-extra/test/clang-tidy/checkers/Inputs/altera-kernel-name-restriction/verilog.h
  
clang-tools-extra/test/clang-tidy/checkers/Inputs/altera-kernel-name-restriction/vhdl.CL
  
clang-tools-extra/test/clang-tidy/checkers/Inputs/altera-kernel-name-restriction/vhdl.h
  
clang-tools-extra/test/clang-tidy/checkers/Inputs/altera-kernel-name-restriction/vhdl_number_two.cl
  clang-tools-extra/test/clang-tidy/checkers/altera-kernel-name-restriction.cpp
  llvm/utils/gn/secondary/clang-tools-extra/clang-tidy/altera/BUILD.gn

Index: llvm/utils/gn/secondary/clang-tools-extra/clang-tidy/altera/BUILD.gn
===
--- llvm/utils/gn/secondary/clang-tools-extra/clang-tidy/altera/BUILD.gn
+++ llvm/utils/gn/secondary/clang-tools-extra/clang-tidy/altera/BUILD.gn
@@ -13,6 +13,7 @@
   ]
   sources = [
 "AlteraTidyModule.cpp",
+"KernelNameRestrictionCheck.cpp",
 "StructPackAlignCheck.cpp",
   ]
 }
Index: clang-tools-extra/test/clang-tidy/checkers/altera-kernel-name-restriction.cpp
===
--- /dev/null
+++ clang-tools-extra/test/clang-tidy/checkers/altera-kernel-name-restriction.cpp
@@ -0,0 +1,42 @@
+// RUN: %check_clang_tidy %s altera-kernel-name-restriction %t -- -- -I%S/Inputs/altera-kernel-name-restriction
+
+// These are the banned kernel filenames, and should trigger warnings
+#include "kernel.cl"
+// CHECK-MESSAGES: :[[@LINE-1]]:1: warning: The imported kernel source file is named 'kernel.cl','Verilog.cl', or 'VHDL.cl', which could cause compilation errors. [altera-kernel-name-restriction]
+#include "Verilog.cl"
+// CHECK-MESSAGES: :[[@LINE-1]]:1: warning: The imported kernel source file is named 'kernel.cl','Verilog.cl', or 'VHDL.cl', which could cause compilation errors. [altera-kernel-name-restriction]
+#include "VHDL.cl"
+// CHECK-MESSAGES: :[[@LINE-1]]:1: warning: The imported kernel source file is named 'kernel.cl','Verilog.cl', or 'VHDL.cl', which could cause compilation errors. [altera-kernel-name-restriction]
+
+// The warning should be triggered regardless of capitalization
+#include "KERNEL.cl"
+// CHECK-MESSAGES: :[[@LINE-1]]:1: warning: The imported kernel source file is named 'kernel.cl','Verilog.cl', or 'VHDL.cl', which could cause compilation errors. [altera-kernel-name-restriction]
+#include "vERILOG.cl"
+// CHECK-MESSAGES: :[[@LINE-1]]:1: warning: The imported kernel source file is named 'kernel.cl','Verilog.cl', or 'VHDL.cl', which could cause compilation errors. [altera-kernel-name-restriction]
+#include "vhdl.CL"
+// CHECK-MESSAGES: :[[@LINE-1]]:1: warning: The imported kernel source file is named 'kernel.cl','Verilog.cl', or 'VHDL.cl', which could cause compilation errors. [altera-kernel-name-restriction]
+
+// The warning should be triggered if the names are within a directory

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

[Frank Derry Wanye via Phabricator via cfe-commits]

ffrankies updated this revision to Diff 254301.
ffrankies added a comment.

- Updated underlying repo to https://github.com/llvm/llvm-project
- Removed braces from one-line if-statements


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

https://reviews.llvm.org/D72218

Files:
  clang-tools-extra/clang-tidy/CMakeLists.txt
  clang-tools-extra/clang-tidy/ClangTidyForceLinker.h
  clang-tools-extra/clang-tidy/altera/AlteraTidyModule.cpp
  clang-tools-extra/clang-tidy/altera/CMakeLists.txt
  clang-tools-extra/clang-tidy/altera/KernelNameRestrictionCheck.cpp
  clang-tools-extra/clang-tidy/altera/KernelNameRestrictionCheck.h
  clang-tools-extra/docs/ReleaseNotes.rst
  clang-tools-extra/docs/clang-tidy/checks/altera-kernel-name-restriction.rst
  clang-tools-extra/docs/clang-tidy/checks/list.rst
  clang-tools-extra/docs/clang-tidy/index.rst
  
clang-tools-extra/test/clang-tidy/checkers/Inputs/altera-kernel-name-restriction/KERNEL.cl
  
clang-tools-extra/test/clang-tidy/checkers/Inputs/altera-kernel-name-restriction/VHDL.cl
  
clang-tools-extra/test/clang-tidy/checkers/Inputs/altera-kernel-name-restriction/Verilog.cl
  
clang-tools-extra/test/clang-tidy/checkers/Inputs/altera-kernel-name-restriction/kernel.cl
  
clang-tools-extra/test/clang-tidy/checkers/Inputs/altera-kernel-name-restriction/kernel.h
  
clang-tools-extra/test/clang-tidy/checkers/Inputs/altera-kernel-name-restriction/other_Verilog.cl
  
clang-tools-extra/test/clang-tidy/checkers/Inputs/altera-kernel-name-restriction/otherdir/vhdl.cl
  
clang-tools-extra/test/clang-tidy/checkers/Inputs/altera-kernel-name-restriction/otherthing.cl
  
clang-tools-extra/test/clang-tidy/checkers/Inputs/altera-kernel-name-restriction/some/dir/kernel.cl
  
clang-tools-extra/test/clang-tidy/checkers/Inputs/altera-kernel-name-restriction/some_kernel.cl
  
clang-tools-extra/test/clang-tidy/checkers/Inputs/altera-kernel-name-restriction/somedir/verilog.cl
  
clang-tools-extra/test/clang-tidy/checkers/Inputs/altera-kernel-name-restriction/thing.h
  
clang-tools-extra/test/clang-tidy/checkers/Inputs/altera-kernel-name-restriction/vERILOG.cl
  
clang-tools-extra/test/clang-tidy/checkers/Inputs/altera-kernel-name-restriction/verilog.h
  
clang-tools-extra/test/clang-tidy/checkers/Inputs/altera-kernel-name-restriction/vhdl.CL
  
clang-tools-extra/test/clang-tidy/checkers/Inputs/altera-kernel-name-restriction/vhdl.h
  
clang-tools-extra/test/clang-tidy/checkers/Inputs/altera-kernel-name-restriction/vhdl_number_two.cl
  clang-tools-extra/test/clang-tidy/checkers/altera-kernel-name-restriction.cpp

Index: clang-tools-extra/test/clang-tidy/checkers/altera-kernel-name-restriction.cpp
===
--- /dev/null
+++ clang-tools-extra/test/clang-tidy/checkers/altera-kernel-name-restriction.cpp
@@ -0,0 +1,42 @@
+// RUN: %check_clang_tidy %s altera-kernel-name-restriction %t -- -- -I%S/Inputs/altera-kernel-name-restriction
+
+// These are the banned kernel filenames, and should trigger warnings
+#include "kernel.cl"
+// CHECK-MESSAGES: :[[@LINE-1]]:1: warning: The imported kernel source file is named 'kernel.cl','Verilog.cl', or 'VHDL.cl', which could cause compilation errors. [altera-kernel-name-restriction]
+#include "Verilog.cl"
+// CHECK-MESSAGES: :[[@LINE-1]]:1: warning: The imported kernel source file is named 'kernel.cl','Verilog.cl', or 'VHDL.cl', which could cause compilation errors. [altera-kernel-name-restriction]
+#include "VHDL.cl"
+// CHECK-MESSAGES: :[[@LINE-1]]:1: warning: The imported kernel source file is named 'kernel.cl','Verilog.cl', or 'VHDL.cl', which could cause compilation errors. [altera-kernel-name-restriction]
+
+// The warning should be triggered regardless of capitalization
+#include "KERNEL.cl"
+// CHECK-MESSAGES: :[[@LINE-1]]:1: warning: The imported kernel source file is named 'kernel.cl','Verilog.cl', or 'VHDL.cl', which could cause compilation errors. [altera-kernel-name-restriction]
+#include "vERILOG.cl"
+// CHECK-MESSAGES: :[[@LINE-1]]:1: warning: The imported kernel source file is named 'kernel.cl','Verilog.cl', or 'VHDL.cl', which could cause compilation errors. [altera-kernel-name-restriction]
+#include "vhdl.CL"
+// CHECK-MESSAGES: :[[@LINE-1]]:1: warning: The imported kernel source file is named 'kernel.cl','Verilog.cl', or 'VHDL.cl', which could cause compilation errors. [altera-kernel-name-restriction]
+
+// The warning should be triggered if the names are within a directory
+#include "some/dir/kernel.cl"
+// CHECK-MESSAGES: :[[@LINE-1]]:1: warning: The imported kernel source file is named 'kernel.cl','Verilog.cl', or 'VHDL.cl', which could cause compilation errors. [altera-kernel-name-restriction]
+#include "somedir/verilog.cl"
+// CHECK-MESSAGES: :[[@LINE-1]]:1: warning: The imported kernel source file is named 'kernel.cl','Verilog.cl', or 'VHDL.cl', which could cause compilation errors. [altera-kernel-name-restriction]
+#include "otherdir/vhdl.cl"
+// CHECK-MESSAGES: 

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

[Frank Derry Wanye via Phabricator via cfe-commits]

ffrankies updated this revision to Diff 243945.
ffrankies marked 9 inline comments as done.
ffrankies edited the summary of this revision.
ffrankies added a comment.

Implemented changes requested by @Eugene.Zelenko:

- Added empty lines around namespace block
- Fixed use of auto keyword
- Fixed formatting in documentation
- Added dependency on previous revision (D66564 
) to the Summary.


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

https://reviews.llvm.org/D72218

Files:
  clang-tidy/CMakeLists.txt
  clang-tidy/ClangTidyForceLinker.h
  clang-tidy/altera/AlteraTidyModule.cpp
  clang-tidy/altera/CMakeLists.txt
  clang-tidy/altera/KernelNameRestrictionCheck.cpp
  clang-tidy/altera/KernelNameRestrictionCheck.h
  docs/ReleaseNotes.rst
  docs/clang-tidy/checks/altera-kernel-name-restriction.rst
  docs/clang-tidy/checks/list.rst
  docs/clang-tidy/index.rst
  test/clang-tidy/checkers/Inputs/altera-kernel-name-restriction/KERNEL.cl
  test/clang-tidy/checkers/Inputs/altera-kernel-name-restriction/VHDL.cl
  test/clang-tidy/checkers/Inputs/altera-kernel-name-restriction/Verilog.cl
  test/clang-tidy/checkers/Inputs/altera-kernel-name-restriction/kernel.cl
  test/clang-tidy/checkers/Inputs/altera-kernel-name-restriction/kernel.h
  
test/clang-tidy/checkers/Inputs/altera-kernel-name-restriction/other_Verilog.cl
  
test/clang-tidy/checkers/Inputs/altera-kernel-name-restriction/otherdir/vhdl.cl
  test/clang-tidy/checkers/Inputs/altera-kernel-name-restriction/otherthing.cl
  
test/clang-tidy/checkers/Inputs/altera-kernel-name-restriction/some/dir/kernel.cl
  test/clang-tidy/checkers/Inputs/altera-kernel-name-restriction/some_kernel.cl
  
test/clang-tidy/checkers/Inputs/altera-kernel-name-restriction/somedir/verilog.cl
  test/clang-tidy/checkers/Inputs/altera-kernel-name-restriction/thing.h
  test/clang-tidy/checkers/Inputs/altera-kernel-name-restriction/vERILOG.cl
  test/clang-tidy/checkers/Inputs/altera-kernel-name-restriction/verilog.h
  test/clang-tidy/checkers/Inputs/altera-kernel-name-restriction/vhdl.CL
  test/clang-tidy/checkers/Inputs/altera-kernel-name-restriction/vhdl.h
  
test/clang-tidy/checkers/Inputs/altera-kernel-name-restriction/vhdl_number_two.cl
  test/clang-tidy/checkers/altera-kernel-name-restriction.cpp

Index: test/clang-tidy/checkers/altera-kernel-name-restriction.cpp
===
--- /dev/null
+++ test/clang-tidy/checkers/altera-kernel-name-restriction.cpp
@@ -0,0 +1,42 @@
+// RUN: %check_clang_tidy %s altera-kernel-name-restriction %t -- -- -I%S/Inputs/altera-kernel-name-restriction
+
+// These are the banned kernel filenames, and should trigger warnings
+#include "kernel.cl"
+// CHECK-MESSAGES: :[[@LINE-1]]:1: warning: The imported kernel source file is named 'kernel.cl','Verilog.cl', or 'VHDL.cl', which could cause compilation errors. [altera-kernel-name-restriction]
+#include "Verilog.cl"
+// CHECK-MESSAGES: :[[@LINE-1]]:1: warning: The imported kernel source file is named 'kernel.cl','Verilog.cl', or 'VHDL.cl', which could cause compilation errors. [altera-kernel-name-restriction]
+#include "VHDL.cl"
+// CHECK-MESSAGES: :[[@LINE-1]]:1: warning: The imported kernel source file is named 'kernel.cl','Verilog.cl', or 'VHDL.cl', which could cause compilation errors. [altera-kernel-name-restriction]
+
+// The warning should be triggered regardless of capitalization
+#include "KERNEL.cl"
+// CHECK-MESSAGES: :[[@LINE-1]]:1: warning: The imported kernel source file is named 'kernel.cl','Verilog.cl', or 'VHDL.cl', which could cause compilation errors. [altera-kernel-name-restriction]
+#include "vERILOG.cl"
+// CHECK-MESSAGES: :[[@LINE-1]]:1: warning: The imported kernel source file is named 'kernel.cl','Verilog.cl', or 'VHDL.cl', which could cause compilation errors. [altera-kernel-name-restriction]
+#include "vhdl.CL"
+// CHECK-MESSAGES: :[[@LINE-1]]:1: warning: The imported kernel source file is named 'kernel.cl','Verilog.cl', or 'VHDL.cl', which could cause compilation errors. [altera-kernel-name-restriction]
+
+// The warning should be triggered if the names are within a directory
+#include "some/dir/kernel.cl"
+// CHECK-MESSAGES: :[[@LINE-1]]:1: warning: The imported kernel source file is named 'kernel.cl','Verilog.cl', or 'VHDL.cl', which could cause compilation errors. [altera-kernel-name-restriction]
+#include "somedir/verilog.cl"
+// CHECK-MESSAGES: :[[@LINE-1]]:1: warning: The imported kernel source file is named 'kernel.cl','Verilog.cl', or 'VHDL.cl', which could cause compilation errors. [altera-kernel-name-restriction]
+#include "otherdir/vhdl.cl"
+// CHECK-MESSAGES: :[[@LINE-1]]:1: warning: The imported kernel source file is named 'kernel.cl','Verilog.cl', or 'VHDL.cl', which could cause compilation errors. [altera-kernel-name-restriction]
+
+// There are no FIX-ITs for the altera-kernel-name-restriction lint check
+
+// The following include directives shouldn't trigger the 

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

[Eugene Zelenko via Phabricator via cfe-commits]

Eugene.Zelenko added inline comments.



Comment at: clang-tidy/altera/KernelNameRestrictionCheck.cpp:13
+#include "clang/Lex/Preprocessor.h"
+
+using namespace clang::ast_matchers;

Please include string, vector



Comment at: clang-tidy/altera/KernelNameRestrictionCheck.cpp:69
+  for (IncludeDirective  : IncludeDirectives) {
+auto FilePath = StringRef(ID.Filename);
+auto FileName = FilePath.substr(FilePath.find_last_of("/\\") + 1);

Please don't use auto unless type is not spelled in same statement or iterator.



Comment at: clang-tidy/altera/KernelNameRestrictionCheck.cpp:70
+auto FilePath = StringRef(ID.Filename);
+auto FileName = FilePath.substr(FilePath.find_last_of("/\\") + 1);
+if (FileName.equals_lower("kernel.cl") ||

Please don't use auto unless type is not spelled in same statement or iterator.



Comment at: clang-tidy/altera/KernelNameRestrictionCheck.cpp:82
+  // Check main file for restricted names.
+  auto Entry = SM.getFileEntryForID(SM.getMainFileID());
+  StringRef FilePath = Entry->getName();

Please don't use auto unless type is not spelled in same statement or iterator.



Comment at: clang-tidy/altera/KernelNameRestrictionCheck.cpp:84
+  StringRef FilePath = Entry->getName();
+  auto FileName = FilePath.substr(FilePath.find_last_of("/\\") + 1);
+  if (FileName.equals_lower("kernel.cl") ||

Please don't use auto unless type is not spelled in same statement or iterator.


Repository:
  rCTE Clang Tools Extra

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

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

[Eugene Zelenko via Phabricator via cfe-commits]

Eugene.Zelenko added inline comments.



Comment at: clang-tidy/altera/KernelNameRestrictionCheck.cpp:21
+namespace {
+class KernelNameRestrictionPPCallbacks : public PPCallbacks {
+public:

Please separate with empty line.



Comment at: clang-tidy/altera/KernelNameRestrictionCheck.cpp:46
+};
+} // namespace
+

Please separate with empty line.



Comment at: docs/ReleaseNotes.rst:79
+
+Checks for cases where the kernel source file is named "kernel.cl",
+  "Verilog.cl", or "VHDL.cl".

Please fix indentation and use single back-ticks to highlight file names. Same 
in documentation.



Comment at: docs/clang-tidy/checks/altera-kernel-name-restriction.rst:13
+
+As per the "Guidelines for Naming the Kernel" section in the "Intel FPGA SDK 
+for OpenCL Pro Edition: Programming Guide."

May be link is better?


Repository:
  rCTE Clang Tools Extra

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

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

[Frank Derry Wanye via Phabricator via cfe-commits]

ffrankies created this revision.
ffrankies added reviewers: alexfh, hokein, aaron.ballman.
ffrankies added projects: clang-tools-extra, clang, LLVM.
Herald added subscribers: mgehre, ormris, arphaman, xazax.hun, Anastasia, 
mgorny.

This lint check is part of the FLOCL (FPGA Linters for OpenCL) project out of 
the Synergy Lab at Virginia Tech.

FLOCL is a set of lint checks aimed at FPGA developers who code in OpenCL.

The altera kernel name restriction check finds kernel files and include 
directives whose filename is "kernel.cl", "Verilog.cl", or "VHDL.cl". Such 
kernel file names cause the Altera Offline Compiler to generate intermediate 
design files that have the same names as certain internal files, which leads to 
a compilation error.

As per the "Guidelines for Naming the Kernel" section in the "Intel FPGA SDK 
for OpenCL Pro Edition: Programming Guide."


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D72218

Files:
  clang-tidy/CMakeLists.txt
  clang-tidy/ClangTidyForceLinker.h
  clang-tidy/altera/AlteraTidyModule.cpp
  clang-tidy/altera/CMakeLists.txt
  clang-tidy/altera/KernelNameRestrictionCheck.cpp
  clang-tidy/altera/KernelNameRestrictionCheck.h
  docs/ReleaseNotes.rst
  docs/clang-tidy/checks/altera-kernel-name-restriction.rst
  docs/clang-tidy/checks/list.rst
  docs/clang-tidy/index.rst
  test/clang-tidy/checkers/Inputs/altera-kernel-name-restriction/KERNEL.cl
  test/clang-tidy/checkers/Inputs/altera-kernel-name-restriction/VHDL.cl
  test/clang-tidy/checkers/Inputs/altera-kernel-name-restriction/Verilog.cl
  test/clang-tidy/checkers/Inputs/altera-kernel-name-restriction/kernel.cl
  test/clang-tidy/checkers/Inputs/altera-kernel-name-restriction/kernel.h
  
test/clang-tidy/checkers/Inputs/altera-kernel-name-restriction/other_Verilog.cl
  
test/clang-tidy/checkers/Inputs/altera-kernel-name-restriction/otherdir/vhdl.cl
  test/clang-tidy/checkers/Inputs/altera-kernel-name-restriction/otherthing.cl
  
test/clang-tidy/checkers/Inputs/altera-kernel-name-restriction/some/dir/kernel.cl
  test/clang-tidy/checkers/Inputs/altera-kernel-name-restriction/some_kernel.cl
  
test/clang-tidy/checkers/Inputs/altera-kernel-name-restriction/somedir/verilog.cl
  test/clang-tidy/checkers/Inputs/altera-kernel-name-restriction/thing.h
  test/clang-tidy/checkers/Inputs/altera-kernel-name-restriction/vERILOG.cl
  test/clang-tidy/checkers/Inputs/altera-kernel-name-restriction/verilog.h
  test/clang-tidy/checkers/Inputs/altera-kernel-name-restriction/vhdl.CL
  test/clang-tidy/checkers/Inputs/altera-kernel-name-restriction/vhdl.h
  
test/clang-tidy/checkers/Inputs/altera-kernel-name-restriction/vhdl_number_two.cl
  test/clang-tidy/checkers/altera-kernel-name-restriction.cpp

Index: test/clang-tidy/checkers/altera-kernel-name-restriction.cpp
===
--- /dev/null
+++ test/clang-tidy/checkers/altera-kernel-name-restriction.cpp
@@ -0,0 +1,42 @@
+// RUN: %check_clang_tidy %s altera-kernel-name-restriction %t -- -- -I%S/Inputs/altera-kernel-name-restriction
+
+// These are the banned kernel filenames, and should trigger warnings
+#include "kernel.cl"
+// CHECK-MESSAGES: :[[@LINE-1]]:1: warning: The imported kernel source file is named 'kernel.cl','Verilog.cl', or 'VHDL.cl', which could cause compilation errors. [altera-kernel-name-restriction]
+#include "Verilog.cl"
+// CHECK-MESSAGES: :[[@LINE-1]]:1: warning: The imported kernel source file is named 'kernel.cl','Verilog.cl', or 'VHDL.cl', which could cause compilation errors. [altera-kernel-name-restriction]
+#include "VHDL.cl"
+// CHECK-MESSAGES: :[[@LINE-1]]:1: warning: The imported kernel source file is named 'kernel.cl','Verilog.cl', or 'VHDL.cl', which could cause compilation errors. [altera-kernel-name-restriction]
+
+// The warning should be triggered regardless of capitalization
+#include "KERNEL.cl"
+// CHECK-MESSAGES: :[[@LINE-1]]:1: warning: The imported kernel source file is named 'kernel.cl','Verilog.cl', or 'VHDL.cl', which could cause compilation errors. [altera-kernel-name-restriction]
+#include "vERILOG.cl"
+// CHECK-MESSAGES: :[[@LINE-1]]:1: warning: The imported kernel source file is named 'kernel.cl','Verilog.cl', or 'VHDL.cl', which could cause compilation errors. [altera-kernel-name-restriction]
+#include "vhdl.CL"
+// CHECK-MESSAGES: :[[@LINE-1]]:1: warning: The imported kernel source file is named 'kernel.cl','Verilog.cl', or 'VHDL.cl', which could cause compilation errors. [altera-kernel-name-restriction]
+
+// The warning should be triggered if the names are within a directory
+#include "some/dir/kernel.cl"
+// CHECK-MESSAGES: :[[@LINE-1]]:1: warning: The imported kernel source file is named 'kernel.cl','Verilog.cl', or 'VHDL.cl', which could cause compilation errors. [altera-kernel-name-restriction]
+#include "somedir/verilog.cl"
+// CHECK-MESSAGES: :[[@LINE-1]]:1: warning: The imported kernel source file is named 'kernel.cl','Verilog.cl', or 'VHDL.cl', which could cause