[PATCH] D74669: [clang-tidy] New check: bugprone-suspicious-include

2020-04-14 Thread Sylvestre Ledru via Phabricator via cfe-commits
sylvestre.ledru added a comment.

Firefox uses a unified build model. For better performances in the binary, the 
C++ files are compiled as the same time from a single file (ex: 
Unified_cpp_netwerk_base3.cpp) which will include the .cpp files.

This isn't a common use case, so, don't hesitate to ignore me but checks 
triggers 4286 new issues. About 20 of them are legit.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D74669



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


[PATCH] D74669: [clang-tidy] New check: bugprone-suspicious-include

2020-03-12 Thread Jonathan Roelofs via Phabricator via cfe-commits
jroelofs added a comment.

re-landed in 
https://github.com/llvm/llvm-project/commit/2c9cf9f4ddd01ae9eb47522266a6343104f9d0b5


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D74669



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


[PATCH] D74669: [clang-tidy] New check: bugprone-suspicious-include

2020-03-10 Thread Aaron Ballman via Phabricator via cfe-commits
aaron.ballman added a comment.

LGTM with the changes for #import.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D74669



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


[PATCH] D74669: [clang-tidy] New check: bugprone-suspicious-include

2020-03-10 Thread Jonathan Roelofs via Phabricator via cfe-commits
jroelofs updated this revision to Diff 249377.
jroelofs added a comment.

Don't fire on `#import`s.

The error on Windows suggests it would normally do the "right" thing anyway:

http://45.33.8.238/win/10088/step_8.txt

  
C:\src\llvm-project\out\gn\obj\clang-tools-extra\test\clang-tidy\checkers\Output\bugprone-suspicious-include.cpp.tmp.cpp:18:2:
 error: #import of type library is an unsupported Microsoft feature 
[clang-diagnostic-error]
  #import "c.c"


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D74669

Files:
  clang-tools-extra/clang-tidy/bugprone/BugproneTidyModule.cpp
  clang-tools-extra/clang-tidy/bugprone/CMakeLists.txt
  clang-tools-extra/clang-tidy/bugprone/SuspiciousIncludeCheck.cpp
  clang-tools-extra/clang-tidy/bugprone/SuspiciousIncludeCheck.h
  clang-tools-extra/clang-tidy/utils/FileExtensionsUtils.cpp
  clang-tools-extra/clang-tidy/utils/FileExtensionsUtils.h
  clang-tools-extra/docs/ReleaseNotes.rst
  clang-tools-extra/docs/clang-tidy/checks/bugprone-suspicious-include.rst
  clang-tools-extra/test/clang-tidy/checkers/Inputs/Headers/a
  clang-tools-extra/test/clang-tidy/checkers/Inputs/Headers/a.cpp
  clang-tools-extra/test/clang-tidy/checkers/Inputs/Headers/a.hpp
  clang-tools-extra/test/clang-tidy/checkers/Inputs/Headers/c.c
  clang-tools-extra/test/clang-tidy/checkers/Inputs/Headers/c.cc
  clang-tools-extra/test/clang-tidy/checkers/Inputs/Headers/c.cxx
  clang-tools-extra/test/clang-tidy/checkers/Inputs/Headers/i.cpp
  clang-tools-extra/test/clang-tidy/checkers/bugprone-suspicious-include.cpp

Index: clang-tools-extra/test/clang-tidy/checkers/bugprone-suspicious-include.cpp
===
--- /dev/null
+++ clang-tools-extra/test/clang-tidy/checkers/bugprone-suspicious-include.cpp
@@ -0,0 +1,24 @@
+// RUN: %check_clang_tidy %s bugprone-suspicious-include %t -- -- -isystem %S/Inputs/Headers -fmodules
+
+// clang-format off
+
+// CHECK-MESSAGES: [[@LINE+4]]:11: warning: suspicious #include of file with '.cpp' extension
+// CHECK-MESSAGES: [[@LINE+3]]:11: note: did you mean to include 'a'?
+// CHECK-MESSAGES: [[@LINE+2]]:11: note: did you mean to include 'a.h'?
+// CHECK-MESSAGES: [[@LINE+1]]:11: note: did you mean to include 'a.hpp'?
+#include "a.cpp"
+
+// CHECK-MESSAGES: [[@LINE+2]]:11: warning: suspicious #include of file with '.cpp' extension
+// CHECK-MESSAGES: [[@LINE+1]]:11: note: did you mean to include 'i.h'?
+#include "i.cpp"
+
+#include "b.h"
+
+// CHECK-MESSAGES: [[@LINE+1]]:16: warning: suspicious #include_next of file with '.c' extension
+#include_next 
+
+// CHECK-MESSAGES: [[@LINE+1]]:13: warning: suspicious #include of file with '.cc' extension
+# include  
+
+// CHECK-MESSAGES: [[@LINE+1]]:14: warning: suspicious #include of file with '.cxx' extension
+#  include  
Index: clang-tools-extra/docs/clang-tidy/checks/bugprone-suspicious-include.rst
===
--- /dev/null
+++ clang-tools-extra/docs/clang-tidy/checks/bugprone-suspicious-include.rst
@@ -0,0 +1,32 @@
+.. title:: clang-tidy - bugprone-suspicious-include
+
+bugprone-suspicious-include
+===
+
+The check detects various cases when an include refers to what appears to be an
+implementation file, which often leads to hard-to-track-down ODR violations.
+
+Examples:
+
+.. code-block:: c++
+
+  #include "Dinosaur.hpp" // OK, .hpp files tend not to have definitions.
+  #include "Pterodactyl.h"// OK, .h files tend not to have definitions.
+  #include "Velociraptor.cpp" // Warning, filename is suspicious.
+  #include_next  // Warning, filename is suspicious.
+
+Options
+---
+.. option:: HeaderFileExtensions
+
+   Default value: `";h;hh;hpp;hxx"`
+   A semicolon-separated list of filename extensions of header files (the
+   filename extensions should not contain a "." prefix). For extension-less
+   header files, use an empty string or leave an empty string between ";"
+   if there are other filename extensions.
+
+.. option:: ImplementationFileExtensions
+
+   Default value: `"c;cc;cpp;cxx"`
+   Likewise, a semicolon-separated list of filename extensions of
+   implementation files.
Index: clang-tools-extra/docs/ReleaseNotes.rst
===
--- clang-tools-extra/docs/ReleaseNotes.rst
+++ clang-tools-extra/docs/ReleaseNotes.rst
@@ -82,6 +82,13 @@
 
   Checks for usages of identifiers reserved for use by the implementation.
 
+- New :doc:`bugprone-suspicious-include
+  ` check.
+
+  Finds cases where an include refers to what appears to be an implementation
+  file, which often leads to hard-to-track-down ODR violations, and diagnoses
+  them.
+
 - New :doc:`cert-oop57-cpp
   ` check.
 
Index: clang-tools-extra/clang-tidy/utils/FileExtensionsUtils.h
===
--- 

[PATCH] D74669: [clang-tidy] New check: bugprone-suspicious-include

2020-03-10 Thread Nico Weber via Phabricator via cfe-commits
thakis added a comment.

Reverted for now in 714466bf367dfd5062e1850179d27c37a9ec6b46 



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D74669



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


[PATCH] D74669: [clang-tidy] New check: bugprone-suspicious-include

2020-03-10 Thread Nico Weber via Phabricator via cfe-commits
thakis added a comment.

Test falls in Windows: http://45.33.8.238/win/10088/step_8.txt


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D74669



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


[PATCH] D74669: [clang-tidy] New check: bugprone-suspicious-include

2020-03-09 Thread Jonathan Roelofs via Phabricator via cfe-commits
jroelofs marked 6 inline comments as done.
jroelofs added a comment.

https://github.com/llvm/llvm-project/commit/52bbdad7d63fd060d102b3591b433d116a982255


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D74669



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


[PATCH] D74669: [clang-tidy] New check: bugprone-suspicious-include

2020-03-09 Thread Eugene Zelenko via Phabricator via cfe-commits
Eugene.Zelenko added inline comments.



Comment at: clang-tools-extra/docs/ReleaseNotes.rst:101
 
+- New :doc:`bugprone-suspicious-includei
+  ` check.

Please keep alphabetical order.



Comment at: clang-tools-extra/docs/ReleaseNotes.rst:104
+
+  Finds includes that appear to be referring to implementation files (which
+  tends to cause ODR violations), and diagnoses them.

Please synchronize with first statement in documentation.



Comment at: 
clang-tools-extra/docs/clang-tidy/checks/bugprone-suspicious-include.rst:6
+
+The checker detects various cases when an include refers to what appears to be
+an implementation file, which often leads to hard-to-track-down ODR violations.

Please omit //The checker//. Clang-tidy uses //check// in its terminology.



Comment at: 
clang-tools-extra/docs/clang-tidy/checks/bugprone-suspicious-include.rst:23
+
+   Default value: ";h;hh;hpp;hxx"
+   A semicolon-separated list of filename extensions of header files (the

Please use single back-ticks for option values. Same above.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D74669



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


[PATCH] D74669: [clang-tidy] New check: bugprone-suspicious-include

2020-03-09 Thread Jonathan Roelofs via Phabricator via cfe-commits
jroelofs updated this revision to Diff 249224.
jroelofs added a comment.

Add missing release notes.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D74669

Files:
  clang-tools-extra/clang-tidy/bugprone/BugproneTidyModule.cpp
  clang-tools-extra/clang-tidy/bugprone/CMakeLists.txt
  clang-tools-extra/clang-tidy/bugprone/SuspiciousIncludeCheck.cpp
  clang-tools-extra/clang-tidy/bugprone/SuspiciousIncludeCheck.h
  clang-tools-extra/clang-tidy/utils/FileExtensionsUtils.cpp
  clang-tools-extra/clang-tidy/utils/FileExtensionsUtils.h
  clang-tools-extra/docs/ReleaseNotes.rst
  clang-tools-extra/docs/clang-tidy/checks/bugprone-suspicious-include.rst
  clang-tools-extra/test/clang-tidy/checkers/Inputs/Headers/a
  clang-tools-extra/test/clang-tidy/checkers/Inputs/Headers/a.cpp
  clang-tools-extra/test/clang-tidy/checkers/Inputs/Headers/a.hpp
  clang-tools-extra/test/clang-tidy/checkers/Inputs/Headers/c.c
  clang-tools-extra/test/clang-tidy/checkers/Inputs/Headers/c.cc
  clang-tools-extra/test/clang-tidy/checkers/Inputs/Headers/c.cxx
  clang-tools-extra/test/clang-tidy/checkers/Inputs/Headers/i.cpp
  clang-tools-extra/test/clang-tidy/checkers/bugprone-suspicious-include.cpp

Index: clang-tools-extra/test/clang-tidy/checkers/bugprone-suspicious-include.cpp
===
--- /dev/null
+++ clang-tools-extra/test/clang-tidy/checkers/bugprone-suspicious-include.cpp
@@ -0,0 +1,27 @@
+// RUN: %check_clang_tidy %s bugprone-suspicious-include %t -- -- -isystem %S/Inputs/Headers -fmodules
+
+// clang-format off
+
+// CHECK-MESSAGES: [[@LINE+4]]:11: warning: suspicious #include of file with '.cpp' extension
+// CHECK-MESSAGES: [[@LINE+3]]:11: note: did you mean to include 'a'?
+// CHECK-MESSAGES: [[@LINE+2]]:11: note: did you mean to include 'a.h'?
+// CHECK-MESSAGES: [[@LINE+1]]:11: note: did you mean to include 'a.hpp'?
+#include "a.cpp"
+
+// CHECK-MESSAGES: [[@LINE+2]]:11: warning: suspicious #include of file with '.cpp' extension
+// CHECK-MESSAGES: [[@LINE+1]]:11: note: did you mean to include 'i.h'?
+#include "i.cpp"
+
+#include "b.h"
+
+// CHECK-MESSAGES: [[@LINE+1]]:10: warning: suspicious #import of file with '.c' extension
+#import "c.c"
+
+// CHECK-MESSAGES: [[@LINE+1]]:16: warning: suspicious #include_next of file with '.c' extension
+#include_next 
+
+// CHECK-MESSAGES: [[@LINE+1]]:13: warning: suspicious #include of file with '.cc' extension
+# include  
+
+// CHECK-MESSAGES: [[@LINE+1]]:14: warning: suspicious #include of file with '.cxx' extension
+#  include  
Index: clang-tools-extra/docs/clang-tidy/checks/bugprone-suspicious-include.rst
===
--- /dev/null
+++ clang-tools-extra/docs/clang-tidy/checks/bugprone-suspicious-include.rst
@@ -0,0 +1,33 @@
+.. title:: clang-tidy - bugprone-suspicious-include
+
+bugprone-suspicious-include
+===
+
+The checker detects various cases when an include refers to what appears to be
+an implementation file, which often leads to hard-to-track-down ODR violations.
+
+Examples:
+
+.. code-block:: c++
+
+  #include "Dinosaur.hpp" // OK, .hpp files tend not to have definitions.
+  #include "Pterodactyl.h"// OK, .h files tend not to have definitions.
+  #include "Velociraptor.cpp" // Warning, filename is suspicious.
+  #import "Stegosaurus.c" // Warning, fliename is suspicious.
+  #include_next  // Warning, filename is suspicious.
+
+Options
+---
+.. option:: HeaderFileExtensions
+
+   Default value: ";h;hh;hpp;hxx"
+   A semicolon-separated list of filename extensions of header files (the
+   filename extensions should not contain a "." prefix). For extension-less
+   header files, use an empty string or leave an empty string between ";"
+   if there are other filename extensions.
+
+.. option:: ImplementationFileExtensions
+
+   Default value: "c;cc;cpp;cxx"
+   Likewise, a semicolon-separated list of filename extensions of
+   implementation files.
Index: clang-tools-extra/docs/ReleaseNotes.rst
===
--- clang-tools-extra/docs/ReleaseNotes.rst
+++ clang-tools-extra/docs/ReleaseNotes.rst
@@ -98,6 +98,12 @@
 
   Finds recursive functions and diagnoses them.
 
+- New :doc:`bugprone-suspicious-includei
+  ` check.
+
+  Finds includes that appear to be referring to implementation files (which
+  tends to cause ODR violations), and diagnoses them.
+
 New check aliases
 ^
 
Index: clang-tools-extra/clang-tidy/utils/FileExtensionsUtils.h
===
--- clang-tools-extra/clang-tidy/utils/FileExtensionsUtils.h
+++ clang-tools-extra/clang-tidy/utils/FileExtensionsUtils.h
@@ -11,6 +11,7 @@
 
 #include "clang/Basic/SourceLocation.h"
 #include "clang/Basic/SourceManager.h"
+#include "llvm/ADT/Optional.h"
 #include 

[PATCH] D74669: [clang-tidy] New check: bugprone-suspicious-include

2020-03-09 Thread Jonathan Roelofs via Phabricator via cfe-commits
jroelofs updated this revision to Diff 249223.
jroelofs added a comment.

Implement review feedback.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D74669

Files:
  clang-tools-extra/clang-tidy/bugprone/BugproneTidyModule.cpp
  clang-tools-extra/clang-tidy/bugprone/CMakeLists.txt
  clang-tools-extra/clang-tidy/bugprone/SuspiciousIncludeCheck.cpp
  clang-tools-extra/clang-tidy/bugprone/SuspiciousIncludeCheck.h
  clang-tools-extra/clang-tidy/utils/FileExtensionsUtils.cpp
  clang-tools-extra/clang-tidy/utils/FileExtensionsUtils.h
  clang-tools-extra/docs/clang-tidy/checks/bugprone-suspicious-include.rst
  clang-tools-extra/test/clang-tidy/checkers/Inputs/Headers/a
  clang-tools-extra/test/clang-tidy/checkers/Inputs/Headers/a.cpp
  clang-tools-extra/test/clang-tidy/checkers/Inputs/Headers/a.hpp
  clang-tools-extra/test/clang-tidy/checkers/Inputs/Headers/c.c
  clang-tools-extra/test/clang-tidy/checkers/Inputs/Headers/c.cc
  clang-tools-extra/test/clang-tidy/checkers/Inputs/Headers/c.cxx
  clang-tools-extra/test/clang-tidy/checkers/Inputs/Headers/i.cpp
  clang-tools-extra/test/clang-tidy/checkers/bugprone-suspicious-include.cpp

Index: clang-tools-extra/test/clang-tidy/checkers/bugprone-suspicious-include.cpp
===
--- /dev/null
+++ clang-tools-extra/test/clang-tidy/checkers/bugprone-suspicious-include.cpp
@@ -0,0 +1,27 @@
+// RUN: %check_clang_tidy %s bugprone-suspicious-include %t -- -- -isystem %S/Inputs/Headers -fmodules
+
+// clang-format off
+
+// CHECK-MESSAGES: [[@LINE+4]]:11: warning: suspicious #include of file with '.cpp' extension
+// CHECK-MESSAGES: [[@LINE+3]]:11: note: did you mean to include 'a'?
+// CHECK-MESSAGES: [[@LINE+2]]:11: note: did you mean to include 'a.h'?
+// CHECK-MESSAGES: [[@LINE+1]]:11: note: did you mean to include 'a.hpp'?
+#include "a.cpp"
+
+// CHECK-MESSAGES: [[@LINE+2]]:11: warning: suspicious #include of file with '.cpp' extension
+// CHECK-MESSAGES: [[@LINE+1]]:11: note: did you mean to include 'i.h'?
+#include "i.cpp"
+
+#include "b.h"
+
+// CHECK-MESSAGES: [[@LINE+1]]:10: warning: suspicious #import of file with '.c' extension
+#import "c.c"
+
+// CHECK-MESSAGES: [[@LINE+1]]:16: warning: suspicious #include_next of file with '.c' extension
+#include_next 
+
+// CHECK-MESSAGES: [[@LINE+1]]:13: warning: suspicious #include of file with '.cc' extension
+# include  
+
+// CHECK-MESSAGES: [[@LINE+1]]:14: warning: suspicious #include of file with '.cxx' extension
+#  include  
Index: clang-tools-extra/docs/clang-tidy/checks/bugprone-suspicious-include.rst
===
--- /dev/null
+++ clang-tools-extra/docs/clang-tidy/checks/bugprone-suspicious-include.rst
@@ -0,0 +1,33 @@
+.. title:: clang-tidy - bugprone-suspicious-include
+
+bugprone-suspicious-include
+===
+
+The checker detects various cases when an include refers to what appears to be
+an implementation file, which often leads to hard-to-track-down ODR violations.
+
+Examples:
+
+.. code-block:: c++
+
+  #include "Dinosaur.hpp" // OK, .hpp files tend not to have definitions.
+  #include "Pterodactyl.h"// OK, .h files tend not to have definitions.
+  #include "Velociraptor.cpp" // Warning, filename is suspicious.
+  #import "Stegosaurus.c" // Warning, fliename is suspicious.
+  #include_next  // Warning, filename is suspicious.
+
+Options
+---
+.. option:: HeaderFileExtensions
+
+   Default value: ";h;hh;hpp;hxx"
+   A semicolon-separated list of filename extensions of header files (the
+   filename extensions should not contain a "." prefix). For extension-less
+   header files, use an empty string or leave an empty string between ";"
+   if there are other filename extensions.
+
+.. option:: ImplementationFileExtensions
+
+   Default value: "c;cc;cpp;cxx"
+   Likewise, a semicolon-separated list of filename extensions of
+   implementation files.
Index: clang-tools-extra/clang-tidy/utils/FileExtensionsUtils.h
===
--- clang-tools-extra/clang-tidy/utils/FileExtensionsUtils.h
+++ clang-tools-extra/clang-tidy/utils/FileExtensionsUtils.h
@@ -11,6 +11,7 @@
 
 #include "clang/Basic/SourceLocation.h"
 #include "clang/Basic/SourceManager.h"
+#include "llvm/ADT/Optional.h"
 #include "llvm/ADT/SmallSet.h"
 #include "llvm/ADT/StringRef.h"
 
@@ -36,6 +37,12 @@
 /// extensions.
 inline StringRef defaultHeaderFileExtensions() { return ";h;hh;hpp;hxx"; }
 
+/// Returns recommended default value for the list of implementaiton file
+/// extensions.
+inline StringRef defaultImplementationFileExtensions() {
+  return "c;cc;cpp;cxx";
+}
+
 /// Returns recommended default value for the list of file extension
 /// delimiters.
 inline StringRef defaultFileExtensionDelimiters() { return ",;"; }
@@ -45,6 +52,11 @@
 

[PATCH] D74669: [clang-tidy] New check: bugprone-suspicious-include

2020-03-09 Thread Jonathan Roelofs via Phabricator via cfe-commits
jroelofs closed this revision.
jroelofs marked 7 inline comments as done.
jroelofs added a comment.

Thanks for the review!

https://github.com/llvm/llvm-project/commit/1e0669bfe05f0f48ee88152c4a1d581f484f8d67




Comment at: 
clang-tools-extra/test/clang-tidy/checkers/bugprone-suspicious-include.cpp:10
+#include "a.cpp"
+
+#include "b.h"

aaron.ballman wrote:
> Can you add a test for:
> ```
> #include "i.cpp"
> ```
> to show that it suggests `i.h` (which already exists in-tree) but not `i` or 
> `i.hpp`?
happy to :)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D74669



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


[PATCH] D74669: [clang-tidy] New check: bugprone-suspicious-include

2020-03-09 Thread 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.

LGTM aside from some documentation and testing nits.




Comment at: 
clang-tools-extra/docs/clang-tidy/checks/bugprone-suspicious-include.rst:15-17
+  #include "Velociraptor.cpp" // Warning, filename is suspicious
+  #import "Stegosaurus.c" // Warning, fliename is suspicious
+  #include_next  // Warning, filename is suspicious

Missing full stops at the end of the comments.



Comment at: 
clang-tools-extra/docs/clang-tidy/checks/bugprone-suspicious-include.rst:24
+   Default value: ";h;hh;hpp;hxx"
+   A semicolon-separated list of filename extensions of header files (The
+   filename extensions should not contain "." prefix). For extension-less

The -> the



Comment at: 
clang-tools-extra/docs/clang-tidy/checks/bugprone-suspicious-include.rst:25
+   A semicolon-separated list of filename extensions of header files (The
+   filename extensions should not contain "." prefix). For extension-less
+   header files, using an empty string or leaving an empty string between ";"

contain "." -> contain a "."



Comment at: 
clang-tools-extra/docs/clang-tidy/checks/bugprone-suspicious-include.rst:26
+   filename extensions should not contain "." prefix). For extension-less
+   header files, using an empty string or leaving an empty string between ";"
+   if there are other filename extensions.

using -> use
leaving -> leave



Comment at: 
clang-tools-extra/docs/clang-tidy/checks/bugprone-suspicious-include.rst:29
+
+.. option:: HeaderFileExtensions
+

ImplementationFileExtensions ;-)



Comment at: 
clang-tools-extra/test/clang-tidy/checkers/bugprone-suspicious-include.cpp:7
+// CHECK-MESSAGES: [[@LINE+3]]:11: note: did you mean to include 'a'?
+// CHECK-MESSAGES: [[@LINE+2]]:11: note: did you mean to include 'a.h'?
+// CHECK-MESSAGES: [[@LINE+1]]:11: note: did you mean to include 'a.hpp'?

For those of you following along at home, `a.h` already exists on ToT (as does 
b.h). :-D



Comment at: 
clang-tools-extra/test/clang-tidy/checkers/bugprone-suspicious-include.cpp:10
+#include "a.cpp"
+
+#include "b.h"

Can you add a test for:
```
#include "i.cpp"
```
to show that it suggests `i.h` (which already exists in-tree) but not `i` or 
`i.hpp`?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D74669



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


[PATCH] D74669: [clang-tidy] New check: bugprone-suspicious-include

2020-03-09 Thread Jonathan Roelofs via Phabricator via cfe-commits
jroelofs updated this revision to Diff 249164.
jroelofs added a comment.

Fix comment, add docs.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D74669

Files:
  clang-tools-extra/clang-tidy/bugprone/BugproneTidyModule.cpp
  clang-tools-extra/clang-tidy/bugprone/CMakeLists.txt
  clang-tools-extra/clang-tidy/bugprone/SuspiciousIncludeCheck.cpp
  clang-tools-extra/clang-tidy/bugprone/SuspiciousIncludeCheck.h
  clang-tools-extra/clang-tidy/utils/FileExtensionsUtils.cpp
  clang-tools-extra/clang-tidy/utils/FileExtensionsUtils.h
  clang-tools-extra/docs/clang-tidy/checks/bugprone-suspicious-include.rst
  clang-tools-extra/test/clang-tidy/checkers/Inputs/Headers/a
  clang-tools-extra/test/clang-tidy/checkers/Inputs/Headers/a.cpp
  clang-tools-extra/test/clang-tidy/checkers/Inputs/Headers/a.hpp
  clang-tools-extra/test/clang-tidy/checkers/Inputs/Headers/c.c
  clang-tools-extra/test/clang-tidy/checkers/Inputs/Headers/c.cc
  clang-tools-extra/test/clang-tidy/checkers/Inputs/Headers/c.cxx
  clang-tools-extra/test/clang-tidy/checkers/bugprone-suspicious-include.cpp

Index: clang-tools-extra/test/clang-tidy/checkers/bugprone-suspicious-include.cpp
===
--- /dev/null
+++ clang-tools-extra/test/clang-tidy/checkers/bugprone-suspicious-include.cpp
@@ -0,0 +1,23 @@
+// RUN: %check_clang_tidy %s bugprone-suspicious-include %t -- -- -isystem %S/Inputs/Headers -fmodules
+
+// clang-format off
+
+// CHECK-MESSAGES: [[@LINE+4]]:11: warning: suspicious #include of file with '.cpp' extension
+// CHECK-MESSAGES: [[@LINE+3]]:11: note: did you mean to include 'a'?
+// CHECK-MESSAGES: [[@LINE+2]]:11: note: did you mean to include 'a.h'?
+// CHECK-MESSAGES: [[@LINE+1]]:11: note: did you mean to include 'a.hpp'?
+#include "a.cpp"
+
+#include "b.h"
+
+// CHECK-MESSAGES: [[@LINE+1]]:10: warning: suspicious #import of file with '.c' extension
+#import "c.c"
+
+// CHECK-MESSAGES: [[@LINE+1]]:16: warning: suspicious #include_next of file with '.c' extension
+#include_next 
+
+// CHECK-MESSAGES: [[@LINE+1]]:13: warning: suspicious #include of file with '.cc' extension
+# include  
+
+// CHECK-MESSAGES: [[@LINE+1]]:14: warning: suspicious #include of file with '.cxx' extension
+#  include  
Index: clang-tools-extra/docs/clang-tidy/checks/bugprone-suspicious-include.rst
===
--- /dev/null
+++ clang-tools-extra/docs/clang-tidy/checks/bugprone-suspicious-include.rst
@@ -0,0 +1,33 @@
+.. title:: clang-tidy - bugprone-suspicious-include
+
+bugprone-suspicious-include
+===
+
+The checker detects various cases when an include refers to what appears to be
+an implementation file, which often leads to hard-to-track-down ODR violations.
+
+Examples:
+
+.. code-block:: c++
+
+  #include "Dinosaur.hpp" // OK, .hpp files tend not to have definitions.
+  #include "Pterodactyl.h"// OK, .h files tend not to have definitions.
+  #include "Velociraptor.cpp" // Warning, filename is suspicious
+  #import "Stegosaurus.c" // Warning, fliename is suspicious
+  #include_next  // Warning, filename is suspicious
+
+Options
+---
+.. option:: HeaderFileExtensions
+
+   Default value: ";h;hh;hpp;hxx"
+   A semicolon-separated list of filename extensions of header files (The
+   filename extensions should not contain "." prefix). For extension-less
+   header files, using an empty string or leaving an empty string between ";"
+   if there are other filename extensions.
+
+.. option:: HeaderFileExtensions
+
+   Default value: "c;cc;cpp;cxx"
+   Likewise, a semicolon-separated list of filename extensions of
+   implementation files.
Index: clang-tools-extra/clang-tidy/utils/FileExtensionsUtils.h
===
--- clang-tools-extra/clang-tidy/utils/FileExtensionsUtils.h
+++ clang-tools-extra/clang-tidy/utils/FileExtensionsUtils.h
@@ -11,6 +11,7 @@
 
 #include "clang/Basic/SourceLocation.h"
 #include "clang/Basic/SourceManager.h"
+#include "llvm/ADT/Optional.h"
 #include "llvm/ADT/SmallSet.h"
 #include "llvm/ADT/StringRef.h"
 
@@ -36,6 +37,12 @@
 /// extensions.
 inline StringRef defaultHeaderFileExtensions() { return ";h;hh;hpp;hxx"; }
 
+/// Returns recommended default value for the list of implementaiton file
+/// extensions.
+inline StringRef defaultImplementationFileExtensions() {
+  return "c;cc;cpp;cxx";
+}
+
 /// Returns recommended default value for the list of file extension
 /// delimiters.
 inline StringRef defaultFileExtensionDelimiters() { return ",;"; }
@@ -45,6 +52,11 @@
  FileExtensionsSet ,
  StringRef Delimiters);
 
+/// Decides whether a file has a header file extension.
+/// Returns the file extension, if included in the provided set.
+llvm::Optional
+getFileExtension(StringRef FileName, const 

[PATCH] D74669: [clang-tidy] New check: bugprone-suspicious-include

2020-03-09 Thread Jonathan Roelofs via Phabricator via cfe-commits
jroelofs marked 3 inline comments as done.
jroelofs added inline comments.



Comment at: clang-tools-extra/clang-tidy/utils/FileExtensionsUtils.h:9
+
+#ifndef LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_UTILS_HEADER_FILE_EXTENSIONS_UTILS_H
+#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_UTILS_HEADER_FILE_EXTENSIONS_UTILS_H

aaron.ballman wrote:
> Can drop the `HEADER` from these macros.
Missed this one before. Oops.
https://github.com/llvm/llvm-project/commit/c71ef7a85d2447903ff9240aff649a57d70b050d


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D74669



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


[PATCH] D74669: [clang-tidy] New check: bugprone-suspicious-include

2020-03-05 Thread Roman Lebedev via Phabricator via cfe-commits
lebedev.ri added inline comments.



Comment at: clang-tools-extra/clang-tidy/utils/FileExtensionsUtils.h:38
+/// extensions.
+inline StringRef defaultHeaderFileExtensions() { return ",h,hh,hpp,hxx"; }
+

njames93 wrote:
> lebedev.ri wrote:
> > njames93 wrote:
> > > A lot of configuration options for clang tidy use semi-colon `;` 
> > > seperated lists. Would it not be wise to carry on the convention and use 
> > > semi colon here (and below) as well. It could break a few peoples 
> > > configuration but that would be realised  fairly quickly and updated
> > I think i'm missing the point of D75621.
> > Why can't we keep using `,` as delimitter?
> > Why do we have to change everything, introduce inconsistency
> > (is it still documented that `,` is still valid?),
> > and spam into terminal if `,` is found?
> This reason
> ```
> ➜  ~ clang-tidy test.cpp --config="{CheckOptions: [{ key: 
> HeaderFileExtensions, value: h,,hpp,hxx }]}" -- -std=c++17
> YAML:1:59: error: unknown key 'hpp'
> {CheckOptions: [{ key: HeaderFileExtensions, value: h,,hpp,hxx }]}
>   ^
> Error: invalid configuration specified.
> Invalid argument
> ➜  ~ clang-tidy test.cpp --config="{CheckOptions: [{ key: 
> HeaderFileExtensions, value: h;;hpp;hxx }]}" -- -std=c++17
> ➜  ~ 
> 
> ```
```
$ clang-tidy test.cpp --config="{CheckOptions: [{ key: HeaderFileExtensions, 
value: h,,hpp,hxx }]}" -- -std=c++17
YAML:1:59: error: unknown key 'hpp'
{CheckOptions: [{ key: HeaderFileExtensions, value: h,,hpp,hxx }]}
  ^
Error: invalid configuration specified.
Invalid argument
$ clang-tidy test.cpp --config="{CheckOptions: [{ key: HeaderFileExtensions, 
value: \"h,,hpp,hxx\" }]}" -- -std=c++17
Error while processing /repositories/llvm-project/test.cpp.
error: no input files [clang-diagnostic-error]
error: no such file or directory: '/repositories/llvm-project/test.cpp' 
[clang-diagnostic-error]
error: unable to handle compilation, expected exactly one compiler job in '' 
[clang-diagnostic-error]
Found compiler error(s).

```


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D74669



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


[PATCH] D74669: [clang-tidy] New check: bugprone-suspicious-include

2020-03-05 Thread Nathan James via Phabricator via cfe-commits
njames93 added inline comments.



Comment at: clang-tools-extra/clang-tidy/utils/FileExtensionsUtils.h:38
+/// extensions.
+inline StringRef defaultHeaderFileExtensions() { return ",h,hh,hpp,hxx"; }
+

lebedev.ri wrote:
> njames93 wrote:
> > A lot of configuration options for clang tidy use semi-colon `;` seperated 
> > lists. Would it not be wise to carry on the convention and use semi colon 
> > here (and below) as well. It could break a few peoples configuration but 
> > that would be realised  fairly quickly and updated
> I think i'm missing the point of D75621.
> Why can't we keep using `,` as delimitter?
> Why do we have to change everything, introduce inconsistency
> (is it still documented that `,` is still valid?),
> and spam into terminal if `,` is found?
This reason
```
➜  ~ clang-tidy test.cpp --config="{CheckOptions: [{ key: HeaderFileExtensions, 
value: h,,hpp,hxx }]}" -- -std=c++17
YAML:1:59: error: unknown key 'hpp'
{CheckOptions: [{ key: HeaderFileExtensions, value: h,,hpp,hxx }]}
  ^
Error: invalid configuration specified.
Invalid argument
➜  ~ clang-tidy test.cpp --config="{CheckOptions: [{ key: HeaderFileExtensions, 
value: h;;hpp;hxx }]}" -- -std=c++17
➜  ~ 

```


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D74669



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


[PATCH] D74669: [clang-tidy] New check: bugprone-suspicious-include

2020-03-05 Thread Roman Lebedev via Phabricator via cfe-commits
lebedev.ri added inline comments.



Comment at: clang-tools-extra/clang-tidy/utils/FileExtensionsUtils.h:38
+/// extensions.
+inline StringRef defaultHeaderFileExtensions() { return ",h,hh,hpp,hxx"; }
+

njames93 wrote:
> A lot of configuration options for clang tidy use semi-colon `;` seperated 
> lists. Would it not be wise to carry on the convention and use semi colon 
> here (and below) as well. It could break a few peoples configuration but that 
> would be realised  fairly quickly and updated
I think i'm missing the point of D75621.
Why can't we keep using `,` as delimitter?
Why do we have to change everything, introduce inconsistency
(is it still documented that `,` is still valid?),
and spam into terminal if `,` is found?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D74669



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


[PATCH] D74669: [clang-tidy] New check: bugprone-suspicious-include

2020-03-04 Thread Jonathan Roelofs via Phabricator via cfe-commits
jroelofs updated this revision to Diff 248246.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D74669

Files:
  clang-tools-extra/clang-tidy/bugprone/BugproneTidyModule.cpp
  clang-tools-extra/clang-tidy/bugprone/CMakeLists.txt
  clang-tools-extra/clang-tidy/bugprone/SuspiciousIncludeCheck.cpp
  clang-tools-extra/clang-tidy/bugprone/SuspiciousIncludeCheck.h
  clang-tools-extra/clang-tidy/utils/FileExtensionsUtils.cpp
  clang-tools-extra/clang-tidy/utils/FileExtensionsUtils.h
  clang-tools-extra/test/clang-tidy/checkers/Inputs/Headers/a
  clang-tools-extra/test/clang-tidy/checkers/Inputs/Headers/a.cpp
  clang-tools-extra/test/clang-tidy/checkers/Inputs/Headers/a.hpp
  clang-tools-extra/test/clang-tidy/checkers/Inputs/Headers/c.c
  clang-tools-extra/test/clang-tidy/checkers/Inputs/Headers/c.cc
  clang-tools-extra/test/clang-tidy/checkers/Inputs/Headers/c.cxx
  clang-tools-extra/test/clang-tidy/checkers/bugprone-suspicious-include.cpp

Index: clang-tools-extra/test/clang-tidy/checkers/bugprone-suspicious-include.cpp
===
--- /dev/null
+++ clang-tools-extra/test/clang-tidy/checkers/bugprone-suspicious-include.cpp
@@ -0,0 +1,23 @@
+// RUN: %check_clang_tidy %s bugprone-suspicious-include %t -- -- -isystem %S/Inputs/Headers -fmodules
+
+// clang-format off
+
+// CHECK-MESSAGES: [[@LINE+4]]:11: warning: suspicious #include of file with '.cpp' extension
+// CHECK-MESSAGES: [[@LINE+3]]:11: note: did you mean to include 'a'?
+// CHECK-MESSAGES: [[@LINE+2]]:11: note: did you mean to include 'a.h'?
+// CHECK-MESSAGES: [[@LINE+1]]:11: note: did you mean to include 'a.hpp'?
+#include "a.cpp"
+
+#include "b.h"
+
+// CHECK-MESSAGES: [[@LINE+1]]:10: warning: suspicious #import of file with '.c' extension
+#import "c.c"
+
+// CHECK-MESSAGES: [[@LINE+1]]:16: warning: suspicious #include_next of file with '.c' extension
+#include_next 
+
+// CHECK-MESSAGES: [[@LINE+1]]:13: warning: suspicious #include of file with '.cc' extension
+# include  
+
+// CHECK-MESSAGES: [[@LINE+1]]:14: warning: suspicious #include of file with '.cxx' extension
+#  include  
Index: clang-tools-extra/clang-tidy/utils/FileExtensionsUtils.h
===
--- clang-tools-extra/clang-tidy/utils/FileExtensionsUtils.h
+++ clang-tools-extra/clang-tidy/utils/FileExtensionsUtils.h
@@ -11,6 +11,7 @@
 
 #include "clang/Basic/SourceLocation.h"
 #include "clang/Basic/SourceManager.h"
+#include "llvm/ADT/Optional.h"
 #include "llvm/ADT/SmallSet.h"
 #include "llvm/ADT/StringRef.h"
 
@@ -36,6 +37,12 @@
 /// extensions.
 inline StringRef defaultHeaderFileExtensions() { return ";h;hh;hpp;hxx"; }
 
+/// Returns recommended default value for the list of implementaiton file
+/// extensions.
+inline StringRef defaultImplementationFileExtensions() {
+  return "c;cc;cpp;cxx";
+}
+
 /// Returns recommended default value for the list of file extension
 /// delimiters.
 inline StringRef defaultFileExtensionDelimiters() { return ",;"; }
@@ -45,6 +52,11 @@
  FileExtensionsSet ,
  StringRef Delimiters);
 
+/// Decides whether a file has a header file extension.
+/// Returns the file extension, if included in the provided set.
+llvm::Optional
+getFileExtension(StringRef FileName, const FileExtensionsSet );
+
 /// Decides whether a file has one of the specified file extensions.
 bool isFileExtension(StringRef FileName,
  const FileExtensionsSet );
Index: clang-tools-extra/clang-tidy/utils/FileExtensionsUtils.cpp
===
--- clang-tools-extra/clang-tidy/utils/FileExtensionsUtils.cpp
+++ clang-tools-extra/clang-tidy/utils/FileExtensionsUtils.cpp
@@ -59,13 +59,20 @@
   return true;
 }
 
-bool isFileExtension(StringRef FileName,
- const FileExtensionsSet ) {
+llvm::Optional
+getFileExtension(StringRef FileName, const FileExtensionsSet ) {
   StringRef Extension = llvm::sys::path::extension(FileName);
   if (Extension.empty())
-return false;
+return llvm::None;
   // Skip "." prefix.
-  return FileExtensions.count(Extension.substr(1)) > 0;
+  if (!FileExtensions.count(Extension.substr(1)))
+return llvm::None;
+  return Extension;
+}
+
+bool isFileExtension(StringRef FileName,
+ const FileExtensionsSet ) {
+  return getFileExtension(FileName, FileExtensions).hasValue();
 }
 
 } // namespace utils
Index: clang-tools-extra/clang-tidy/bugprone/SuspiciousIncludeCheck.h
===
--- /dev/null
+++ clang-tools-extra/clang-tidy/bugprone/SuspiciousIncludeCheck.h
@@ -0,0 +1,57 @@
+//===--- SuspiciousIncludeCheck.h - clang-tidy --*- C++ -*-===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See 

[PATCH] D74669: [clang-tidy] New check: bugprone-suspicious-include

2020-03-04 Thread Jonathan Roelofs via Phabricator via cfe-commits
jroelofs updated this revision to Diff 248208.
jroelofs marked an inline comment as not done.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D74669

Files:
  clang-tools-extra/clang-tidy/bugprone/BugproneTidyModule.cpp
  clang-tools-extra/clang-tidy/bugprone/CMakeLists.txt
  clang-tools-extra/clang-tidy/bugprone/SuspiciousIncludeCheck.cpp
  clang-tools-extra/clang-tidy/bugprone/SuspiciousIncludeCheck.h
  clang-tools-extra/clang-tidy/utils/FileExtensionsUtils.cpp
  clang-tools-extra/clang-tidy/utils/FileExtensionsUtils.h
  clang-tools-extra/test/clang-tidy/checkers/Inputs/Headers/a
  clang-tools-extra/test/clang-tidy/checkers/Inputs/Headers/a.cpp
  clang-tools-extra/test/clang-tidy/checkers/Inputs/Headers/a.hpp
  clang-tools-extra/test/clang-tidy/checkers/Inputs/Headers/c.c
  clang-tools-extra/test/clang-tidy/checkers/Inputs/Headers/c.cc
  clang-tools-extra/test/clang-tidy/checkers/Inputs/Headers/c.cxx
  clang-tools-extra/test/clang-tidy/checkers/bugprone-suspicious-include.cpp

Index: clang-tools-extra/test/clang-tidy/checkers/bugprone-suspicious-include.cpp
===
--- /dev/null
+++ clang-tools-extra/test/clang-tidy/checkers/bugprone-suspicious-include.cpp
@@ -0,0 +1,21 @@
+// RUN: %check_clang_tidy %s bugprone-suspicious-include %t -- -- -isystem %S/Inputs/Headers -fmodules
+
+// CHECK-MESSAGES: [[@LINE+4]]:11: warning: suspicious #include of file with '.cpp' extension
+// CHECK-MESSAGES: [[@LINE+3]]:11: note: did you mean to include 'a'?
+// CHECK-MESSAGES: [[@LINE+2]]:11: note: did you mean to include 'a.h'?
+// CHECK-MESSAGES: [[@LINE+1]]:11: note: did you mean to include 'a.hpp'?
+#include "a.cpp"
+
+#include "b.h"
+
+// CHECK-MESSAGES: [[@LINE+1]]:10: warning: suspicious #import of file with '.c' extension
+#import "c.c"
+
+// CHECK-MESSAGES: [[@LINE+1]]:16: warning: suspicious #include_next of file with '.c' extension
+#include_next 
+
+// CHECK-MESSAGES: [[@LINE+1]]:13: warning: suspicious #include of file with '.cc' extension
+# include  
+
+// CHECK-MESSAGES: [[@LINE+1]]:14: warning: suspicious #include of file with '.cxx' extension
+#  include  
Index: clang-tools-extra/clang-tidy/utils/FileExtensionsUtils.h
===
--- clang-tools-extra/clang-tidy/utils/FileExtensionsUtils.h
+++ clang-tools-extra/clang-tidy/utils/FileExtensionsUtils.h
@@ -11,6 +11,7 @@
 
 #include "clang/Basic/SourceLocation.h"
 #include "clang/Basic/SourceManager.h"
+#include "llvm/ADT/Optional.h"
 #include "llvm/ADT/SmallSet.h"
 #include "llvm/ADT/StringRef.h"
 
@@ -36,10 +37,21 @@
 /// extensions.
 inline StringRef defaultHeaderFileExtensions() { return ";h;hh;hpp;hxx"; }
 
+/// Returns recommended default value for the list of implementaiton file
+/// extensions.
+inline StringRef defaultImplementationFileExtensions() {
+  return "c;cc;cpp;cxx";
+}
+
 /// Parses header file extensions from a semicolon-separated list.
 bool parseFileExtensions(StringRef AllFileExtensions,
  FileExtensionsSet , char Delimiter);
 
+/// Decides whether a file has a header file extension.
+/// Returns the file extension, if included in the provided set.
+llvm::Optional
+getFileExtension(StringRef FileName, const FileExtensionsSet );
+
 /// Decides whether a file has one of the specified file extensions.
 bool isFileExtension(StringRef FileName,
  const FileExtensionsSet );
Index: clang-tools-extra/clang-tidy/utils/FileExtensionsUtils.cpp
===
--- clang-tools-extra/clang-tidy/utils/FileExtensionsUtils.cpp
+++ clang-tools-extra/clang-tidy/utils/FileExtensionsUtils.cpp
@@ -46,13 +46,20 @@
   return true;
 }
 
-bool isFileExtension(StringRef FileName,
- const FileExtensionsSet ) {
+llvm::Optional
+getFileExtension(StringRef FileName, const FileExtensionsSet ) {
   StringRef Extension = llvm::sys::path::extension(FileName);
   if (Extension.empty())
-return false;
+return llvm::None;
   // Skip "." prefix.
-  return FileExtensions.count(Extension.substr(1)) > 0;
+  if (!FileExtensions.count(Extension.substr(1)))
+return llvm::None;
+  return Extension;
+}
+
+bool isFileExtension(StringRef FileName,
+ const FileExtensionsSet ) {
+  return getFileExtension(FileName, FileExtensions).hasValue();
 }
 
 } // namespace utils
Index: clang-tools-extra/clang-tidy/bugprone/SuspiciousIncludeCheck.h
===
--- /dev/null
+++ clang-tools-extra/clang-tidy/bugprone/SuspiciousIncludeCheck.h
@@ -0,0 +1,57 @@
+//===--- SuspiciousIncludeCheck.h - clang-tidy --*- C++ -*-===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// 

[PATCH] D74669: [clang-tidy] New check: bugprone-suspicious-include

2020-03-04 Thread Jonathan Roelofs via Phabricator via cfe-commits
jroelofs updated this revision to Diff 248203.
jroelofs added a comment.

Split out https://reviews.llvm.org/D75621


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D74669

Files:
  clang-tools-extra/clang-tidy/bugprone/BugproneTidyModule.cpp
  clang-tools-extra/clang-tidy/bugprone/CMakeLists.txt
  clang-tools-extra/clang-tidy/bugprone/SuspiciousIncludeCheck.cpp
  clang-tools-extra/clang-tidy/bugprone/SuspiciousIncludeCheck.h
  clang-tools-extra/clang-tidy/utils/FileExtensionsUtils.cpp
  clang-tools-extra/clang-tidy/utils/FileExtensionsUtils.h
  clang-tools-extra/test/clang-tidy/checkers/Inputs/Headers/a
  clang-tools-extra/test/clang-tidy/checkers/Inputs/Headers/a.cpp
  clang-tools-extra/test/clang-tidy/checkers/Inputs/Headers/a.hpp
  clang-tools-extra/test/clang-tidy/checkers/Inputs/Headers/c.c
  clang-tools-extra/test/clang-tidy/checkers/Inputs/Headers/c.cc
  clang-tools-extra/test/clang-tidy/checkers/Inputs/Headers/c.cxx
  clang-tools-extra/test/clang-tidy/checkers/bugprone-suspicious-include.cpp

Index: clang-tools-extra/test/clang-tidy/checkers/bugprone-suspicious-include.cpp
===
--- /dev/null
+++ clang-tools-extra/test/clang-tidy/checkers/bugprone-suspicious-include.cpp
@@ -0,0 +1,21 @@
+// RUN: %check_clang_tidy %s bugprone-suspicious-include %t -- -- -isystem %S/Inputs/Headers -fmodules
+
+// CHECK-MESSAGES: [[@LINE+4]]:11: warning: suspicious #include of file with '.cpp' extension
+// CHECK-MESSAGES: [[@LINE+3]]:11: note: did you mean to include 'a'?
+// CHECK-MESSAGES: [[@LINE+2]]:11: note: did you mean to include 'a.h'?
+// CHECK-MESSAGES: [[@LINE+1]]:11: note: did you mean to include 'a.hpp'?
+#include "a.cpp"
+
+#include "b.h"
+
+// CHECK-MESSAGES: [[@LINE+1]]:10: warning: suspicious #import of file with '.c' extension
+#import "c.c"
+
+// CHECK-MESSAGES: [[@LINE+1]]:16: warning: suspicious #include_next of file with '.c' extension
+#include_next 
+
+// CHECK-MESSAGES: [[@LINE+1]]:13: warning: suspicious #include of file with '.cc' extension
+# include  
+
+// CHECK-MESSAGES: [[@LINE+1]]:14: warning: suspicious #include of file with '.cxx' extension
+#  include  
Index: clang-tools-extra/clang-tidy/utils/FileExtensionsUtils.h
===
--- clang-tools-extra/clang-tidy/utils/FileExtensionsUtils.h
+++ clang-tools-extra/clang-tidy/utils/FileExtensionsUtils.h
@@ -11,6 +11,7 @@
 
 #include "clang/Basic/SourceLocation.h"
 #include "clang/Basic/SourceManager.h"
+#include "llvm/ADT/Optional.h"
 #include "llvm/ADT/SmallSet.h"
 #include "llvm/ADT/StringRef.h"
 
@@ -36,10 +37,21 @@
 /// extensions.
 inline StringRef defaultHeaderFileExtensions() { return ";h;hh;hpp;hxx"; }
 
+/// Returns recommended default value for the list of implementaiton file
+/// extensions.
+inline StringRef defaultImplementationFileExtensions() {
+  return "c;cc;cpp;cxx";
+}
+
 /// Parses header file extensions from a semicolon-separated list.
 bool parseFileExtensions(StringRef AllFileExtensions,
  FileExtensionsSet , char Delimiter);
 
+/// Decides whether a file has a header file extension.
+/// Returns the file extension, if included in the provided set.
+llvm::Optional
+getFileExtension(StringRef FileName, const FileExtensionsSet );
+
 /// Decides whether a file has one of the specified file extensions.
 bool isFileExtension(StringRef FileName,
  const FileExtensionsSet );
Index: clang-tools-extra/clang-tidy/utils/FileExtensionsUtils.cpp
===
--- clang-tools-extra/clang-tidy/utils/FileExtensionsUtils.cpp
+++ clang-tools-extra/clang-tidy/utils/FileExtensionsUtils.cpp
@@ -46,13 +46,20 @@
   return true;
 }
 
-bool isFileExtension(StringRef FileName,
- const FileExtensionsSet ) {
+llvm::Optional
+getFileExtension(StringRef FileName, const FileExtensionsSet ) {
   StringRef Extension = llvm::sys::path::extension(FileName);
   if (Extension.empty())
-return false;
+return llvm::None;
   // Skip "." prefix.
-  return FileExtensions.count(Extension.substr(1)) > 0;
+  if (!FileExtensions.count(Extension.substr(1)))
+return llvm::None;
+  return Extension;
+}
+
+bool isFileExtension(StringRef FileName,
+ const FileExtensionsSet ) {
+  return getFileExtension(FileName, FileExtensions).hasValue();
 }
 
 } // namespace utils
Index: clang-tools-extra/clang-tidy/bugprone/SuspiciousIncludeCheck.h
===
--- /dev/null
+++ clang-tools-extra/clang-tidy/bugprone/SuspiciousIncludeCheck.h
@@ -0,0 +1,57 @@
+//===--- SuspiciousIncludeCheck.h - clang-tidy --*- C++ -*-===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license 

[PATCH] D74669: [clang-tidy] New check: bugprone-suspicious-include

2020-03-02 Thread Jonathan Roelofs via Phabricator via cfe-commits
jroelofs marked 2 inline comments as done.
jroelofs added a comment.

In D74669#1902107 , @njames93 wrote:

> Adding the parent revision means you don't need to have those changes in this 
> patch.


IIUC, that is what I've already done:

https://reviews.llvm.org/D74669 is `git show c4dd6f5903e -U999`
https://reviews.llvm.org/D75489 is `git show 0cda7e0b9ed -U999`

where `c4dd6f5903e` is the parent of `0cda7e0b9ed`.

The suggestion to switch over to `;`s as delimiters is what's causing this diff 
to still touch so many files.




Comment at: 
clang-tools-extra/clang-tidy/bugprone/DynamicStaticInitializersCheck.h:24
+/// extensions of header files (The filename extensions should not contain
+/// "." prefix). "h;hh;hpp;hxx" by default.
+//

";h;hh;hpp;hxx"



Comment at: 
clang-tools-extra/clang-tidy/google/UnnamedNamespaceInHeaderCheck.h:25
+/// extensions of header files (The filename extensions should not contain
+/// "." prefix). "h;hh;hpp;hxx" by default.
+///

";h;hh;hpp;hxx"


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D74669



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


[PATCH] D74669: [clang-tidy] New check: bugprone-suspicious-include

2020-03-02 Thread Nathan James via Phabricator via cfe-commits
njames93 added a comment.

Adding the parent revision means you don't need to have those changes in this 
patch. It will cause issues down the line. best thing is to run a diff from 
this to the parent and just include those changes


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D74669



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


[PATCH] D74669: [clang-tidy] New check: bugprone-suspicious-include

2020-03-02 Thread Jonathan Roelofs via Phabricator via cfe-commits
jroelofs updated this revision to Diff 247764.
jroelofs added a comment.

implement review feedback


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D74669

Files:
  clang-tools-extra/clang-tidy/bugprone/BugproneTidyModule.cpp
  clang-tools-extra/clang-tidy/bugprone/CMakeLists.txt
  clang-tools-extra/clang-tidy/bugprone/DynamicStaticInitializersCheck.cpp
  clang-tools-extra/clang-tidy/bugprone/DynamicStaticInitializersCheck.h
  clang-tools-extra/clang-tidy/bugprone/SuspiciousIncludeCheck.cpp
  clang-tools-extra/clang-tidy/bugprone/SuspiciousIncludeCheck.h
  clang-tools-extra/clang-tidy/google/GlobalNamesInHeadersCheck.cpp
  clang-tools-extra/clang-tidy/google/GlobalNamesInHeadersCheck.h
  clang-tools-extra/clang-tidy/google/UnnamedNamespaceInHeaderCheck.cpp
  clang-tools-extra/clang-tidy/google/UnnamedNamespaceInHeaderCheck.h
  clang-tools-extra/clang-tidy/llvm/HeaderGuardCheck.h
  clang-tools-extra/clang-tidy/misc/DefinitionsInHeadersCheck.cpp
  clang-tools-extra/clang-tidy/misc/DefinitionsInHeadersCheck.h
  clang-tools-extra/clang-tidy/utils/FileExtensionsUtils.cpp
  clang-tools-extra/clang-tidy/utils/FileExtensionsUtils.h
  clang-tools-extra/clang-tidy/utils/HeaderGuard.h
  clang-tools-extra/test/clang-tidy/checkers/Inputs/Headers/a
  clang-tools-extra/test/clang-tidy/checkers/Inputs/Headers/a.cpp
  clang-tools-extra/test/clang-tidy/checkers/Inputs/Headers/a.hpp
  clang-tools-extra/test/clang-tidy/checkers/Inputs/Headers/c.c
  clang-tools-extra/test/clang-tidy/checkers/Inputs/Headers/c.cc
  clang-tools-extra/test/clang-tidy/checkers/Inputs/Headers/c.cxx
  clang-tools-extra/test/clang-tidy/checkers/bugprone-suspicious-include.cpp

Index: clang-tools-extra/test/clang-tidy/checkers/bugprone-suspicious-include.cpp
===
--- /dev/null
+++ clang-tools-extra/test/clang-tidy/checkers/bugprone-suspicious-include.cpp
@@ -0,0 +1,21 @@
+// RUN: %check_clang_tidy %s bugprone-suspicious-include %t -- -- -isystem %S/Inputs/Headers -fmodules
+
+// CHECK-MESSAGES: [[@LINE+4]]:11: warning: suspicious #include of file with '.cpp' extension
+// CHECK-MESSAGES: [[@LINE+3]]:11: note: did you mean to include 'a'?
+// CHECK-MESSAGES: [[@LINE+2]]:11: note: did you mean to include 'a.h'?
+// CHECK-MESSAGES: [[@LINE+1]]:11: note: did you mean to include 'a.hpp'?
+#include "a.cpp"
+
+#include "b.h"
+
+// CHECK-MESSAGES: [[@LINE+1]]:10: warning: suspicious #import of file with '.c' extension
+#import "c.c"
+
+// CHECK-MESSAGES: [[@LINE+1]]:16: warning: suspicious #include_next of file with '.c' extension
+#include_next 
+
+// CHECK-MESSAGES: [[@LINE+1]]:13: warning: suspicious #include of file with '.cc' extension
+# include  
+
+// CHECK-MESSAGES: [[@LINE+1]]:14: warning: suspicious #include of file with '.cxx' extension
+#  include  
Index: clang-tools-extra/clang-tidy/utils/HeaderGuard.h
===
--- clang-tools-extra/clang-tidy/utils/HeaderGuard.h
+++ clang-tools-extra/clang-tidy/utils/HeaderGuard.h
@@ -18,11 +18,12 @@
 
 /// Finds and fixes header guards.
 /// The check supports these options:
-///   - `HeaderFileExtensions`: a comma-separated list of filename extensions of
-/// header files (The filename extension should not contain "." prefix).
-/// ",h,hh,hpp,hxx" by default.
+///   - `HeaderFileExtensions`: a semicolon-separated list of filename
+/// extensions of header files (The filename extension should not contain
+/// "." prefix). ";h;hh;hpp;hxx" by default.
+///
 /// For extension-less header files, using an empty string or leaving an
-/// empty string between "," if there are other filename extensions.
+/// empty string between ";" if there are other filename extensions.
 class HeaderGuardCheck : public ClangTidyCheck {
 public:
   HeaderGuardCheck(StringRef Name, ClangTidyContext *Context)
@@ -30,7 +31,7 @@
 RawStringHeaderFileExtensions(Options.getLocalOrGlobal(
 "HeaderFileExtensions", utils::defaultHeaderFileExtensions())) {
 utils::parseFileExtensions(RawStringHeaderFileExtensions,
-   HeaderFileExtensions, ',');
+   HeaderFileExtensions, ';');
   }
   void registerPPCallbacks(const SourceManager , Preprocessor *PP,
Preprocessor *ModuleExpanderPP) override;
Index: clang-tools-extra/clang-tidy/utils/FileExtensionsUtils.h
===
--- clang-tools-extra/clang-tidy/utils/FileExtensionsUtils.h
+++ clang-tools-extra/clang-tidy/utils/FileExtensionsUtils.h
@@ -11,6 +11,7 @@
 
 #include "clang/Basic/SourceLocation.h"
 #include "clang/Basic/SourceManager.h"
+#include "llvm/ADT/Optional.h"
 #include "llvm/ADT/SmallSet.h"
 #include "llvm/ADT/StringRef.h"
 
@@ -34,12 +35,23 @@
 
 /// Returns recommended default value for the list