[PATCH] D74669: [clang-tidy] New check: misc-no-include-cpp

[Nathan James via Phabricator via cfe-commits]

njames93 added inline comments.



Comment at: clang-tools-extra/clang-tidy/bugprone/SuspiciousIncludeCheck.cpp:44
+  RawStringRecommendedExtensions(Options.getLocalOrGlobal(
+  "RecommendedExtensions", utils::defaultHeaderFileExtensions())) {
+  if (!utils::parseFileExtensions(RawStringSuspiciousExtensions,

Follow the convention of the other checks and use `HeaderFileExtensions` as the 
option name. Probably call the other option `ImplementationFileExtensions`



Comment at: clang-tools-extra/clang-tidy/utils/FileExtensionsUtils.cpp:42
+StringRef Extension = Suffix.trim();
+for (StringRef::const_iterator it = Extension.begin();
+ it != Extension.end(); ++it) {

This could be changed to 
```
if (!llvm::all_of(Extension, isAlphanumeric))
  return false;
```



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

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


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: misc-no-include-cpp

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.



Comment at: clang-tools-extra/clang-tidy/bugprone/SuspiciousIncludeCheck.cpp:76
+
+  const auto SE = utils::getFileExtension(FileName, 
Check.SuspiciousExtensions);
+  if (!SE)

Please don't use `auto` here (the type isn't spelled out in the initialization).



Comment at: clang-tools-extra/clang-tidy/utils/FileExtensionsUtils.cpp:36
+bool parseFileExtensions(StringRef AllFileExtensions,
+ FileExtensionsSet &FileExtensions, char delimiter) {
+  SmallVector Suffixes;

`Delimiter` here as well.



Comment at: clang-tools-extra/clang-tidy/utils/FileExtensionsUtils.cpp:54
+getFileExtension(StringRef FileName, const FileExtensionsSet &FileExtensions) {
+  StringRef extension = llvm::sys::path::extension(FileName);
+  if (extension.empty())

`Extension`



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

Can drop the `HEADER` from these macros.



Comment at: clang-tools-extra/clang-tidy/utils/FileExtensionsUtils.h:48
+bool parseFileExtensions(StringRef AllFileExtensions,
+ FileExtensionsSet &FileExtensions, char delimiter);
+

delimiter -> Delimiter per our usual naming conventions.


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: misc-no-include-cpp

[Eugene Zelenko via Phabricator via cfe-commits]

Eugene.Zelenko added a comment.

In D74669#1892577 , @jroelofs wrote:

> Implement review feedback:
>
> - Re-purpose `HeaderFileExtensionsUtils.h` to support headers *and* sources.
> - Surround file extension in diagnostic with `''`s.
>
> I have these as two separate patches locally, but I don't know how to 
> represent that in phabricator, so I've uploaded the diff across both. If this 
> gets approved, I intend to push them as separate commits.


You could merge two patches (diff to base version) and update diff in this 
request.


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: misc-no-include-cpp

[Nathan James via Phabricator via cfe-commits]

njames93 added a comment.

In D74669#1892577 , @jroelofs wrote:

> Implement review feedback:
>
> - Re-purpose `HeaderFileExtensionsUtils.h` to support headers *and* sources.
> - Surround file extension in diagnostic with `''`s.
>
> I have these as two separate patches locally, but I don't know how to 
> represent that in phabricator, so I've uploaded the diff across both. If this 
> gets approved, I intend to push them as separate commits.


You can create a review that just re-purposes HeaderFileExtensionsUtils then 
set that review as a parent of this review


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: misc-no-include-cpp

[Jonathan Roelofs via Phabricator via cfe-commits]

jroelofs updated this revision to Diff 246604.
jroelofs added a comment.

Implement review feedback:

- Re-purpose `HeaderFileExtensionsUtils.h` to support headers *and* sources.
- Surround file extension in diagnostic with `''`s.

I have these as two separate patches locally, but I don't know how to represent 
that in phabricator, so I've uploaded the diff across both. If this gets 
approved, I intend to push them as separate commits.


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/misc/DefinitionsInHeadersCheck.cpp
  clang-tools-extra/clang-tidy/misc/DefinitionsInHeadersCheck.h
  clang-tools-extra/clang-tidy/utils/CMakeLists.txt
  clang-tools-extra/clang-tidy/utils/FileExtensionsUtils.cpp
  clang-tools-extra/clang-tidy/utils/FileExtensionsUtils.h
  clang-tools-extra/clang-tidy/utils/HeaderFileExtensionsUtils.cpp
  clang-tools-extra/clang-tidy/utils/HeaderFileExtensionsUtils.h
  clang-tools-extra/clang-tidy/utils/HeaderGuard.cpp
  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
@@ -10,7 +10,7 @@
 #define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_UTILS_HEADERGUARD_H
 
 #include "../ClangTidy.h"
-#include "../utils/HeaderFileExtensionsUtils.h"
+#include "../utils/FileExtensionsUtils.h"
 
 namespace clang {
 namespace tidy {
@@ -29,8 +29,8 @@
   : ClangTidyCheck(Name, Context),
 RawStringHeaderFileExtensions(Options.getLocalOrGlobal(
 "HeaderFileExtensions", utils::defaultHeaderFileExtensions())) {
-utils::parseHeaderFileExtensions(RawStringHeaderFileExtensions,
- HeaderFileExtensions, ',');
+utils::parseFileExtensions(RawStringHeaderFileExtensions,
+   HeaderFileExtensions, ',');
   }
   void registerPPCallbacks(const SourceManager &SM, Preprocessor *PP,
Preprocessor *ModuleExpanderPP) override;
@@ -54,7 +54,7 @@
 
 private:
   std::string RawStringHeaderFileExtensions;
-  utils::HeaderFileExtensionsSet HeaderFileExtensions;
+  utils::FileExtensionsSet HeaderFileExtensions;
 };
 
 } // namespace utils
Index: clang-tools-extra/clang-tidy/utils/HeaderGuard.cpp
===
--- clang-tools-extra/clang-tidy/utils/HeaderGuard.cpp
+++ clang-tools-extra/clang-tidy/utils/HeaderGuard.cpp
@@ -273,13 +273,13 @@
 }
 
 bool HeaderGuardCheck::shouldSuggestEndifComment(StringRef Fi

[PATCH] D74669: [clang-tidy] New check: misc-no-include-cpp

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.



Comment at: 
clang-tools-extra/clang-tidy/bugprone/SuspiciousIncludeCheck.cpp:50-51
+
+  const char *const SuspiciousExtensions[] = {".c", ".cpp", ".cxx", ".cc"};
+  const char *const RecommendedExtensions[] = {"", ".h", ".hpp", ".hh"};
+

We already have `HeaderFileExtensionsUtils.h` -- we should be using that for 
the header file extensions (which should be configurable). 

It might make sense to make those utilities be general for other kinds of file 
extensions as well, such as source files. I think that list should also be 
configurable.



Comment at: clang-tools-extra/clang-tidy/bugprone/SuspiciousIncludeCheck.cpp:55
+if (FileName.consume_back(SE)) {
+  Check.diag(DiagLoc, "suspicious #%0 of file with %1 extension")
+  << IncludeTok.getIdentifierInfo()->getName() << SE;

Should probably add `'` quotes around the extension to make it more obvious 
where the extension starts and stops.


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: misc-no-include-cpp

[Nathan James via Phabricator via cfe-commits]

njames93 added a comment.

I'd suggest possibly adding 2 Options at Global Level SourceFileExtensions and 
HeaderFileExtensions, both would take semicolon seperated lists. 
Reason they are Global is there are probably other checks that could use them.
The SourceFileExtensions could be defaulted to `.c;.cc;.cpp;.cxx` and the 
HeaderFileExtensions `.h;.hh;.hpp;`.
You need the `` because how Options get parsed, then it can manually be 
transformed to `""` when you read it.


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: misc-no-include-cpp

[Jonathan Roelofs via Phabricator via cfe-commits]

jroelofs updated this revision to Diff 245031.
jroelofs added a comment.

Make the diagnostics more accurate.


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/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/bugprone/SuspiciousIncludeCheck.h
===
--- /dev/null
+++ clang-tools-extra/clang-tidy/bugprone/SuspiciousIncludeCheck.h
@@ -0,0 +1,39 @@
+//===--- 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.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===--===//
+
+#ifndef LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_BUGPRONE_SUSPICIOUSINCLUDECHECK_H
+#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_BUGPRONE_SUSPICIOUSINCLUDECHECK_H
+
+#include "../ClangTidyCheck.h"
+
+namespace clang {
+namespace tidy {
+namespace bugprone {
+
+/// Warns on inclusion of files whose names suggest that they're implementation
+/// files, instead of headers. E.g:
+///
+/// #include "foo.cpp"  // warning
+/// #include "bar.c"// warning
+/// #include "baz.h"// no diagnostic
+///
+/// For the user-facing documentation see:
+/// http://clang.llvm.org/extra/clang-tidy/checks/bugprone-suspicious-include.html
+class SuspiciousIncludeCheck : public ClangTidyCheck {
+public:
+  SuspiciousIncludeCheck(StringRef Name, ClangTidyContext *Context)
+  : ClangTidyCheck(Name, Context) {}
+  void registerPPCallbacks(const SourceManager &SM, Preprocessor *PP,
+   Preprocessor *ModuleExpanderPP) override;
+};
+
+} // namespace bugprone
+} // namespace tidy
+} // namespace clang
+
+#endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_BUGPRONE_SUSPICIOUSINCLUDECHECK_H
Index: clang-tools-extra/clang-tidy/bugprone/SuspiciousIncludeCheck.cpp
===
--- /dev/null
+++ clang-tools-extra/clang-tidy/bugprone/SuspiciousIncludeCheck.cpp
@@ -0,0 +1,75 @@
+//===--- SuspiciousIncludeCheck.cpp - clang-tidy --===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===--===//
+
+#include "SuspiciousIncludeCheck.h"
+#include "clang/AST/ASTContext.h"
+
+namespace clang {
+namespace tidy {
+namespace bugprone {
+
+namespace {
+class SuspiciousIncludePPCallbacks : public PPCallbacks {
+public:
+  explicit SuspiciousIncludePPCallbacks(ClangTidyCheck &Check,
+const SourceManager &SM,
+Preprocessor *PP)
+  : Check(Check), PP(PP) {}
+
+  void InclusionDirective(SourceLocation HashLoc, const Token &IncludeTok,
+  StringRef FileName, bool IsAngled

[PATCH] D74669: [clang-tidy] New check: misc-no-include-cpp

[Jonathan Roelofs via Phabricator via cfe-commits]

jroelofs marked an inline comment as done.
jroelofs added inline comments.



Comment at: clang-tools-extra/clang-tidy/bugprone/SuspiciousIncludeCheck.cpp:59
+Optional File = PP->LookupFile(
+HashLoc /* FIXME: lies */, (FileName + RE).str(), IsAngled, 
nullptr,
+nullptr, CurDir, nullptr, nullptr, nullptr, nullptr, nullptr);

njames93 wrote:
> jroelofs wrote:
> > FIXME: is this a reasonable `SourceLocation` to give to `LookupFile`?
> Main thing is to make sure that the same location is used for the warnings 
> and the notes.
> I personally found this the most pleasing diagnostics
> 
> ```
> SourceLocation DiagLoc = FilenameRange.getBegin().getLocWithOffset(1);
> ```
> 
> ```
> /home/nathan/src/llvm-project/build/Release/tools/clang/tools/extra/test/clang-tidy/checkers/Output/bugprone-suspicious-include.cpp.tmp.cpp:7:11:
>  warning: suspicious #include of file with .cpp extension 
> [bugprone-suspicious-include]
> #include "a.cpp"
>   ^
> /home/nathan/src/llvm-project/build/Release/tools/clang/tools/extra/test/clang-tidy/checkers/Output/bugprone-suspicious-include.cpp.tmp.cpp:7:11:
>  note: did you mean to include 'a'?
> /home/nathan/src/llvm-project/build/Release/tools/clang/tools/extra/test/clang-tidy/checkers/Output/bugprone-suspicious-include.cpp.tmp.cpp:7:11:
>  note: did you mean to include 'a.h'?
> /home/nathan/src/llvm-project/build/Release/tools/clang/tools/extra/test/clang-tidy/checkers/Output/bugprone-suspicious-include.cpp.tmp.cpp:7:11:
>  note: did you mean to include 'a.hpp'?
> ```
I don't have much experience with modules to lean on for intuition on what this 
check should do with them. I've added a test to codify what the pass currently 
does in that case, but if there's something better it should be trying when 
searching for suggestions, let's make it do that instead.


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: misc-no-include-cpp

[Jonathan Roelofs via Phabricator via cfe-commits]

jroelofs updated this revision to Diff 244994.
jroelofs added a comment.

Implement review feedback re: DiagLoc


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/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 #include of file with .c extension
+#import "c.c"
+
+// CHECK-MESSAGES: [[@LINE+1]]:16: warning: suspicious #include 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/bugprone/SuspiciousIncludeCheck.h
===
--- /dev/null
+++ clang-tools-extra/clang-tidy/bugprone/SuspiciousIncludeCheck.h
@@ -0,0 +1,39 @@
+//===--- 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.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===--===//
+
+#ifndef LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_BUGPRONE_SUSPICIOUSINCLUDECHECK_H
+#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_BUGPRONE_SUSPICIOUSINCLUDECHECK_H
+
+#include "../ClangTidyCheck.h"
+
+namespace clang {
+namespace tidy {
+namespace bugprone {
+
+/// Warns on inclusion of files whose names suggest that they're implementation
+/// files, instead of headers. E.g:
+///
+/// #include "foo.cpp"  // warning
+/// #include "bar.c"// warning
+/// #include "baz.h"// no diagnostic
+///
+/// For the user-facing documentation see:
+/// http://clang.llvm.org/extra/clang-tidy/checks/bugprone-suspicious-include.html
+class SuspiciousIncludeCheck : public ClangTidyCheck {
+public:
+  SuspiciousIncludeCheck(StringRef Name, ClangTidyContext *Context)
+  : ClangTidyCheck(Name, Context) {}
+  void registerPPCallbacks(const SourceManager &SM, Preprocessor *PP,
+   Preprocessor *ModuleExpanderPP) override;
+};
+
+} // namespace bugprone
+} // namespace tidy
+} // namespace clang
+
+#endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_BUGPRONE_SUSPICIOUSINCLUDECHECK_H
Index: clang-tools-extra/clang-tidy/bugprone/SuspiciousIncludeCheck.cpp
===
--- /dev/null
+++ clang-tools-extra/clang-tidy/bugprone/SuspiciousIncludeCheck.cpp
@@ -0,0 +1,75 @@
+//===--- SuspiciousIncludeCheck.cpp - clang-tidy --===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===--===//
+
+#include "SuspiciousIncludeCheck.h"
+#include "clang/AST/ASTContext.h"
+
+namespace clang {
+namespace tidy {
+namespace bugprone {
+
+namespace {
+class SuspiciousIncludePPCallbacks : public PPCallbacks {
+public:
+  explicit SuspiciousIncludePPCallbacks(ClangTidyCheck &Check,
+const SourceManager &SM,
+Preprocessor *PP)
+  : Check(Check), PP(PP) {}
+
+  void InclusionDirective(SourceLocation HashLoc, const Token &IncludeTok,
+  

[PATCH] D74669: [clang-tidy] New check: misc-no-include-cpp

[Nathan James via Phabricator via cfe-commits]

njames93 added inline comments.



Comment at: clang-tools-extra/clang-tidy/bugprone/SuspiciousIncludeCheck.cpp:43
+
+void SuspiciousIncludePPCallbacks::InclusionDirective(
+SourceLocation HashLoc, const Token &IncludeTok, StringRef FileName,

This function is used for `#import` on modules. Do we still want the same 
behaviour when importing modules? More thinking about the extension suggestions 
with that one. 



Comment at: clang-tools-extra/clang-tidy/bugprone/SuspiciousIncludeCheck.cpp:59
+Optional File = PP->LookupFile(
+HashLoc /* FIXME: lies */, (FileName + RE).str(), IsAngled, 
nullptr,
+nullptr, CurDir, nullptr, nullptr, nullptr, nullptr, nullptr);

jroelofs wrote:
> FIXME: is this a reasonable `SourceLocation` to give to `LookupFile`?
Main thing is to make sure that the same location is used for the warnings and 
the notes.
I personally found this the most pleasing diagnostics

```
SourceLocation DiagLoc = FilenameRange.getBegin().getLocWithOffset(1);
```

```
/home/nathan/src/llvm-project/build/Release/tools/clang/tools/extra/test/clang-tidy/checkers/Output/bugprone-suspicious-include.cpp.tmp.cpp:7:11:
 warning: suspicious #include of file with .cpp extension 
[bugprone-suspicious-include]
#include "a.cpp"
  ^
/home/nathan/src/llvm-project/build/Release/tools/clang/tools/extra/test/clang-tidy/checkers/Output/bugprone-suspicious-include.cpp.tmp.cpp:7:11:
 note: did you mean to include 'a'?
/home/nathan/src/llvm-project/build/Release/tools/clang/tools/extra/test/clang-tidy/checkers/Output/bugprone-suspicious-include.cpp.tmp.cpp:7:11:
 note: did you mean to include 'a.h'?
/home/nathan/src/llvm-project/build/Release/tools/clang/tools/extra/test/clang-tidy/checkers/Output/bugprone-suspicious-include.cpp.tmp.cpp:7:11:
 note: did you mean to include 'a.hpp'?
```


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: misc-no-include-cpp

[Jonathan Roelofs via Phabricator via cfe-commits]

jroelofs marked an inline comment as done.
jroelofs added inline comments.



Comment at: clang-tools-extra/clang-tidy/bugprone/SuspiciousIncludeCheck.cpp:59
+Optional File = PP->LookupFile(
+HashLoc /* FIXME: lies */, (FileName + RE).str(), IsAngled, 
nullptr,
+nullptr, CurDir, nullptr, nullptr, nullptr, nullptr, nullptr);

FIXME: is this a reasonable `SourceLocation` to give to `LookupFile`?


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: misc-no-include-cpp

[Jonathan Roelofs via Phabricator via cfe-commits]

jroelofs updated this revision to Diff 244884.
jroelofs added a comment.

git format-patch


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/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,18 @@
+// RUN: %check_clang_tidy %s bugprone-suspicious-include %t -- -- -isystem %S/Inputs/Headers
+
+// CHECK-MESSAGES: [[@LINE+4]]:1: warning: suspicious #include of file with .cpp extension
+// CHECK-MESSAGES: [[@LINE+3]]:1: note: did you mean to include 'a'?
+// CHECK-MESSAGES: [[@LINE+2]]:1: note: did you mean to include 'a.h'?
+// CHECK-MESSAGES: [[@LINE+1]]:1: note: did you mean to include 'a.hpp'?
+#include "a.cpp"
+
+#include "b.h"
+
+// CHECK-MESSAGES: [[@LINE+1]]:1: warning: suspicious #include of file with .c extension
+#include 
+
+// CHECK-MESSAGES: [[@LINE+1]]:1: warning: suspicious #include of file with .cc extension
+#include 
+
+// CHECK-MESSAGES: [[@LINE+1]]:1: warning: suspicious #include of file with .cxx extension
+#include 
Index: clang-tools-extra/clang-tidy/bugprone/SuspiciousIncludeCheck.h
===
--- /dev/null
+++ clang-tools-extra/clang-tidy/bugprone/SuspiciousIncludeCheck.h
@@ -0,0 +1,39 @@
+//===--- 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.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===--===//
+
+#ifndef LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_BUGPRONE_SUSPICIOUSINCLUDECHECK_H
+#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_BUGPRONE_SUSPICIOUSINCLUDECHECK_H
+
+#include "../ClangTidyCheck.h"
+
+namespace clang {
+namespace tidy {
+namespace bugprone {
+
+/// Warns on inclusion of files whose names suggest that they're implementation
+/// files, instead of headers. E.g:
+///
+/// #include "foo.cpp"  // warning
+/// #include "bar.c"// warning
+/// #include "baz.h"// no diagnostic
+///
+/// For the user-facing documentation see:
+/// http://clang.llvm.org/extra/clang-tidy/checks/bugprone-suspicious-include.html
+class SuspiciousIncludeCheck : public ClangTidyCheck {
+public:
+  SuspiciousIncludeCheck(StringRef Name, ClangTidyContext *Context)
+  : ClangTidyCheck(Name, Context) {}
+  void registerPPCallbacks(const SourceManager &SM, Preprocessor *PP,
+   Preprocessor *ModuleExpanderPP) override;
+};
+
+} // namespace bugprone
+} // namespace tidy
+} // namespace clang
+
+#endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_BUGPRONE_SUSPICIOUSINCLUDECHECK_H
Index: clang-tools-extra/clang-tidy/bugprone/SuspiciousIncludeCheck.cpp
===
--- /dev/null
+++ clang-tools-extra/clang-tidy/bugprone/SuspiciousIncludeCheck.cpp
@@ -0,0 +1,73 @@
+//===--- SuspiciousIncludeCheck.cpp - clang-tidy --===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===--===//
+
+#include "SuspiciousIncludeCheck.h"
+#include "clang/AST/ASTContext.h"
+
+namespace clang {
+namespace tidy {
+namespace bugprone {
+
+namespace {
+class SuspiciousIncludePPCallbacks : public PPCallbacks {
+public:
+  explicit SuspiciousIncludePPCallbacks(ClangTidyCheck &Check,
+const SourceManager &SM,
+Preprocessor *PP)
+  : Check(Check), PP(PP) {}
+
+  void InclusionDirective(SourceLocation HashLoc, const Token &IncludeTok,
+  StringRef FileName, bool IsAngled,
+  CharSourceRange FilenameRange, const FileEntry *File,
+  StringRef SearchPath, StringRef RelativePath,
+ 

[PATCH] D74669: [clang-tidy] New check: misc-no-include-cpp

[Jonathan Roelofs via Phabricator via cfe-commits]

jroelofs marked an inline comment as done.
jroelofs added inline comments.



Comment at: clang-tools-extra/clang-tidy/bugprone/SuspiciousIncludeCheck.h:26
+/// For the user-facing documentation see:
+/// 
http://clang.llvm.org/extra/clang-tidy/checks/bugprone-suspicious-include.html
+class SuspiciousIncludeCheck : public ClangTidyCheck {

FIXME: still need to write docs for this.



Comment at: clang-tools-extra/clang-tidy/misc/NoIncludeCPPCheck.cpp:50
+
+  (void)FilenameRange;
+  (void)File;

njames93 wrote:
> I have a feeling this is to suppress some unused parameter linting check. If 
> it is then it should be removed as unused parameters in an overridden 
> function shouldn't emit a warning.
> Same for below.
> 
> Side note: File a bug with whichever linter tool gave you that warning (if 
> there was one).
> 
Sorry, did this out of habit. Indeed it isn't actually needed.


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: misc-no-include-cpp

[Jonathan Roelofs via Phabricator via cfe-commits]

jroelofs marked 4 inline comments as done and an inline comment as not done.
jroelofs added inline comments.



Comment at: clang-tools-extra/clang-tidy/misc/NoIncludeCPPCheck.cpp:62
+  Check.diag(HashLoc, "suspicious #include")
+  << FixItHint::CreateReplacement(FilenameRange,
+  ((IsAngled ? "<" : "\"") + FileName +

njames93 wrote:
> jroelofs wrote:
> > njames93 wrote:
> > > This replacement is dangerous, I have a feeling no fix-it should be 
> > > provided or at least do a search of the include directories to see if 
> > > file you are trying to include actually does exist. The correct file 
> > > could be `*.hpp` like what boost uses for all its header files
> > Yeah, perhaps the FixIt should only be added if there is a single candidate 
> > replacement that exists on the `-I` path.
> > 
> > Another option is to not add FixIts at all, and instead emit a list of 
> > `note:`s suggesting each of the candidates.
> How about the case if someone wants to (for whatever reason) include 
> something like this:
> 
> ```
> #include "example.h"
> #include "example.cpp"
> ```
> That looks intentional and a fix it shouldn't be emitted.
I'd imagine that people doing this intentionally (unity builds come to mind) 
would turn off this check. That said, is there something better this check 
could be doing to accommodate those people?


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: misc-no-include-cpp

[Jonathan Roelofs via Phabricator via cfe-commits]

jroelofs updated this revision to Diff 244883.
jroelofs marked an inline comment as done.
jroelofs added a comment.

- Renamed to bugprone-suspicious-include
- Removed FixIts in favor of `note:` suggestions, iff the suggested file exists 
on the header search path.
- Removed unnecessary unused parameter casts-to-void.


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/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,19 @@
+// RUN: %check_clang_tidy %s bugprone-suspicious-include %t -- -- -isystem %S/Inputs/Headers
+
+// CHECK-MESSAGES: [[@LINE+4]]:1: warning: suspicious #include of file with .cpp extension
+// CHECK-MESSAGES: [[@LINE+3]]:1: note: did you mean to include 'a'?
+// CHECK-MESSAGES: [[@LINE+2]]:1: note: did you mean to include 'a.h'?
+// CHECK-MESSAGES: [[@LINE+1]]:1: note: did you mean to include 'a.hpp'?
+#include "a.cpp"
+
+#include "b.h"
+
+// CHECK-MESSAGES: [[@LINE+1]]:1: warning: suspicious #include of file with .c extension
+#include 
+
+// CHECK-MESSAGES: [[@LINE+1]]:1: warning: suspicious #include of file with .cc extension
+#include 
+
+// CHECK-MESSAGES: [[@LINE+1]]:1: warning: suspicious #include of file with .cxx extension
+#include 
+
Index: clang-tools-extra/clang-tidy/bugprone/SuspiciousIncludeCheck.h
===
--- /dev/null
+++ clang-tools-extra/clang-tidy/bugprone/SuspiciousIncludeCheck.h
@@ -0,0 +1,39 @@
+//===--- 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.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===--===//
+
+#ifndef LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_BUGPRONE_SUSPICIOUSINCLUDECHECK_H
+#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_BUGPRONE_SUSPICIOUSINCLUDECHECK_H
+
+#include "../ClangTidyCheck.h"
+
+namespace clang {
+namespace tidy {
+namespace bugprone {
+
+/// Warns on inclusion of files whose names suggest that they're implementation
+/// files, instead of headers. E.g:
+///
+/// #include "foo.cpp"  // warning
+/// #include "bar.c"// warning
+/// #include "baz.h"// no diagnostic
+///
+/// For the user-facing documentation see:
+/// http://clang.llvm.org/extra/clang-tidy/checks/bugprone-suspicious-include.html
+class SuspiciousIncludeCheck : public ClangTidyCheck {
+public:
+  SuspiciousIncludeCheck(StringRef Name, ClangTidyContext *Context)
+  : ClangTidyCheck(Name, Context) {}
+  void registerPPCallbacks(const SourceManager &SM, Preprocessor *PP,
+   Preprocessor *ModuleExpanderPP) override;
+};
+
+} // namespace bugprone
+} // namespace tidy
+} // namespace clang
+
+#endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_BUGPRONE_SUSPICIOUSINCLUDECHECK_H
Index: clang-tools-extra/clang-tidy/bugprone/SuspiciousIncludeCheck.cpp
===
--- /dev/null
+++ clang-tools-extra/clang-tidy/bugprone/SuspiciousIncludeCheck.cpp
@@ -0,0 +1,74 @@
+//===--- SuspiciousIncludeCheck.cpp - clang-tidy --===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===--===//
+
+#include "SuspiciousIncludeCheck.h"
+#include "clang/AST/ASTContext.h"
+
+namespace clang {
+namespace tidy {
+namespace bugprone {
+
+namespace {
+class SuspiciousIncludePPCallbacks : public PPCallbacks {
+public:
+  explicit SuspiciousIncludePPCallbacks(ClangTidyCheck &Check,
+const SourceManager &SM,
+Preprocessor *PP)
+  : Check(Check), PP(PP) {}
+
+  void InclusionDirecti

[PATCH] D74669: [clang-tidy] New check: misc-no-include-cpp

[Nathan James via Phabricator via cfe-commits]

njames93 added inline comments.



Comment at: clang-tools-extra/clang-tidy/misc/NoIncludeCPPCheck.cpp:11-13
+#include "clang/ASTMatchers/ASTMatchFinder.h"
+
+using namespace clang::ast_matchers;

You don't need to include or use the AST matchers in a preprocessor only check.



Comment at: clang-tools-extra/clang-tidy/misc/NoIncludeCPPCheck.cpp:50
+
+  (void)FilenameRange;
+  (void)File;

I have a feeling this is to suppress some unused parameter linting check. If it 
is then it should be removed as unused parameters in an overridden function 
shouldn't emit a warning.
Same for below.

Side note: File a bug with whichever linter tool gave you that warning (if 
there was one).




Comment at: clang-tools-extra/clang-tidy/misc/NoIncludeCPPCheck.cpp:62
+  Check.diag(HashLoc, "suspicious #include")
+  << FixItHint::CreateReplacement(FilenameRange,
+  ((IsAngled ? "<" : "\"") + FileName +

jroelofs wrote:
> njames93 wrote:
> > This replacement is dangerous, I have a feeling no fix-it should be 
> > provided or at least do a search of the include directories to see if file 
> > you are trying to include actually does exist. The correct file could be 
> > `*.hpp` like what boost uses for all its header files
> Yeah, perhaps the FixIt should only be added if there is a single candidate 
> replacement that exists on the `-I` path.
> 
> Another option is to not add FixIts at all, and instead emit a list of 
> `note:`s suggesting each of the candidates.
How about the case if someone wants to (for whatever reason) include something 
like this:

```
#include "example.h"
#include "example.cpp"
```
That looks intentional and a fix it shouldn't be emitted.



Comment at: clang-tools-extra/clang-tidy/misc/NoIncludeCPPCheck.h:31
+  : ClangTidyCheck(Name, Context) {}
+  void registerPPCallbacks(const SourceManager &SM, Preprocessor *PP,
+   Preprocessor *ModuleExpanderPP);

This should be marked `override`.


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: misc-no-include-cpp

[Jonathan Roelofs via Phabricator via cfe-commits]

jroelofs marked an inline comment as done.
jroelofs added a comment.

In D74669#1878168 , @njames93 wrote:

> I have a feeling this check should be called something along the lines of 
> bugprone-suspicous-include.


That's a much better name, I like it.




Comment at: clang-tools-extra/clang-tidy/misc/NoIncludeCPPCheck.cpp:62
+  Check.diag(HashLoc, "suspicious #include")
+  << FixItHint::CreateReplacement(FilenameRange,
+  ((IsAngled ? "<" : "\"") + FileName +

njames93 wrote:
> This replacement is dangerous, I have a feeling no fix-it should be provided 
> or at least do a search of the include directories to see if file you are 
> trying to include actually does exist. The correct file could be `*.hpp` like 
> what boost uses for all its header files
Yeah, perhaps the FixIt should only be added if there is a single candidate 
replacement that exists on the `-I` path.

Another option is to not add FixIts at all, and instead emit a list of `note:`s 
suggesting each of the candidates.


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: misc-no-include-cpp

[Nathan James via Phabricator via cfe-commits]

njames93 added a comment.

I have a feeling this check should be called something along the lines of 
bugprone-suspicous-include.




Comment at: clang-tools-extra/clang-tidy/misc/NoIncludeCPPCheck.cpp:62
+  Check.diag(HashLoc, "suspicious #include")
+  << FixItHint::CreateReplacement(FilenameRange,
+  ((IsAngled ? "<" : "\"") + FileName +

This replacement is dangerous, I have a feeling no fix-it should be provided or 
at least do a search of the include directories to see if file you are trying 
to include actually does exist. The correct file could be `*.hpp` like what 
boost uses for all its header files


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: misc-no-include-cpp

[Eugene Zelenko via Phabricator via cfe-commits]

Eugene.Zelenko added a comment.
Herald added a subscriber: wuzish.

How about .cc and .cxx extension? It'll be reasonable to provide option to 
configure list of extensions.

Please add documentation and mention new check in Release Notes.




Comment at: clang-tools-extra/clang-tidy/misc/NoIncludeCPPCheck.cpp:49
+SrcMgr::CharacteristicKind FileType) {
+
+  (void)FilenameRange;

Unnecessary empty line.


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: misc-no-include-cpp

[Jonathan Roelofs via Phabricator via cfe-commits]

jroelofs created this revision.
jroelofs added a reviewer: aaron.ballman.
Herald added subscribers: cfe-commits, kbarton, xazax.hun, mgorny, nemanjai.
Herald added a project: clang.

Detects and fixes suspicious code like: `#include "foo.cpp"`.

  

Inspired by: https://twitter.com/lefticus/status/1228458240364687360?s=20


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D74669

Files:
  clang-tools-extra/clang-tidy/misc/CMakeLists.txt
  clang-tools-extra/clang-tidy/misc/MiscTidyModule.cpp
  clang-tools-extra/clang-tidy/misc/NoIncludeCPPCheck.cpp
  clang-tools-extra/clang-tidy/misc/NoIncludeCPPCheck.h
  clang-tools-extra/test/clang-tidy/checkers/Inputs/Headers/a.cpp
  clang-tools-extra/test/clang-tidy/checkers/Inputs/Headers/c.c
  clang-tools-extra/test/clang-tidy/checkers/misc-no-include-cpp.cpp

Index: clang-tools-extra/test/clang-tidy/checkers/misc-no-include-cpp.cpp
===
--- /dev/null
+++ clang-tools-extra/test/clang-tidy/checkers/misc-no-include-cpp.cpp
@@ -0,0 +1,11 @@
+// RUN: %check_clang_tidy %s misc-no-include-cpp %t -- -- -isystem %S/Inputs/Headers
+
+// CHECK-MESSAGES: [[@LINE+2]]:1: warning: suspicious #include
+// CHECK-MESSAGES: [[@LINE+3]]:1: warning: suspicious #include
+#include "a.cpp"
+#include "b.h"
+#include 
+
+// CHECK-FIXES: #include "a.h"
+// CHECK-FIXES-NEXT: #include "b.h"
+// CHECK-FIXES-NEXT: #include 
Index: clang-tools-extra/clang-tidy/misc/NoIncludeCPPCheck.h
===
--- /dev/null
+++ clang-tools-extra/clang-tidy/misc/NoIncludeCPPCheck.h
@@ -0,0 +1,39 @@
+//===--- NoIncludeCPPCheck.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.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===--===//
+
+#ifndef LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_MISC_NOINCLUDECPPCHECK_H
+#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_MISC_NOINCLUDECPPCHECK_H
+
+#include "../ClangTidyCheck.h"
+
+namespace clang {
+namespace tidy {
+namespace misc {
+
+/// Warns on inclusion of files whose names suggest that they're implementation
+/// files, instead of headers. E.g:
+///
+/// #include "foo.cpp"  // warning
+/// #include "bar.c"// warning
+/// #include "baz.h"// no diagnostic
+///
+/// For the user-facing documentation see:
+/// http://clang.llvm.org/extra/clang-tidy/checks/misc-NoIncludeCPP.html
+class NoIncludeCPPCheck : public ClangTidyCheck {
+public:
+  NoIncludeCPPCheck(StringRef Name, ClangTidyContext *Context)
+  : ClangTidyCheck(Name, Context) {}
+  void registerPPCallbacks(const SourceManager &SM, Preprocessor *PP,
+   Preprocessor *ModuleExpanderPP);
+};
+
+} // namespace misc
+} // namespace tidy
+} // namespace clang
+
+#endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_MISC_NOINCLUDECPPCHECK_H
Index: clang-tools-extra/clang-tidy/misc/NoIncludeCPPCheck.cpp
===
--- /dev/null
+++ clang-tools-extra/clang-tidy/misc/NoIncludeCPPCheck.cpp
@@ -0,0 +1,70 @@
+//===--- NoIncludeCPPCheck.cpp - clang-tidy ---===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===--===//
+
+#include "NoIncludeCPPCheck.h"
+#include "clang/AST/ASTContext.h"
+#include "clang/ASTMatchers/ASTMatchFinder.h"
+
+using namespace clang::ast_matchers;
+
+namespace clang {
+namespace tidy {
+namespace misc {
+
+namespace {
+class NoIncludeCCallbacks : public PPCallbacks {
+public:
+  explicit NoIncludeCCallbacks(ClangTidyCheck &Check,
+   const SourceManager &SM)
+  : Check(Check) {}
+
+  void InclusionDirective(SourceLocation HashLoc, const Token &IncludeTok,
+  StringRef FileName, bool IsAngled,
+  CharSourceRange FilenameRange, const FileEntry *File,
+  StringRef SearchPath, StringRef RelativePath,
+  const Module *Imported,
+  SrcMgr::CharacteristicKind FileType) override;
+
+private:
+  ClangTidyCheck &Check;
+};
+} // namespace
+
+void NoIncludeCPPCheck::registerPPCallbacks(const SourceManager &SM,
+Preprocessor *PP,
+Preprocessor *ModuleExpanderPP) {
+  PP->addPPCallbacks(::std::make_unique(*this, SM));
+}
+
+void NoIncludeCCallbacks::InclusionDirective(
+SourceLocation HashLoc, const Token &IncludeTok, StringRef FileName,
+bool IsAngl