[PATCH] D54349: [clang-tidy] new check 'readability-redundant-preprocessor'

[Miklos Vajna via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rL350922: [clang-tidy] new check 
readability-redundant-preprocessor (authored by vmiklos, committed 
by ).
Herald added a subscriber: llvm-commits.

Changed prior to commit:
  https://reviews.llvm.org/D54349?vs=174439=181214#toc

Repository:
  rL LLVM

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

https://reviews.llvm.org/D54349

Files:
  clang-tools-extra/trunk/clang-tidy/readability/CMakeLists.txt
  clang-tools-extra/trunk/clang-tidy/readability/ReadabilityTidyModule.cpp
  clang-tools-extra/trunk/clang-tidy/readability/RedundantPreprocessorCheck.cpp
  clang-tools-extra/trunk/clang-tidy/readability/RedundantPreprocessorCheck.h
  clang-tools-extra/trunk/docs/ReleaseNotes.rst
  clang-tools-extra/trunk/docs/clang-tidy/checks/list.rst
  
clang-tools-extra/trunk/docs/clang-tidy/checks/readability-redundant-preprocessor.rst
  
clang-tools-extra/trunk/test/clang-tidy/readability-redundant-preprocessor-ifdef.cpp
  clang-tools-extra/trunk/test/clang-tidy/readability-redundant-preprocessor.cpp
  clang-tools-extra/trunk/test/clang-tidy/readability-redundant-preprocessor.h

Index: clang-tools-extra/trunk/clang-tidy/readability/CMakeLists.txt
===
--- clang-tools-extra/trunk/clang-tidy/readability/CMakeLists.txt
+++ clang-tools-extra/trunk/clang-tidy/readability/CMakeLists.txt
@@ -24,6 +24,7 @@
   RedundantDeclarationCheck.cpp
   RedundantFunctionPtrDereferenceCheck.cpp
   RedundantMemberInitCheck.cpp
+  RedundantPreprocessorCheck.cpp
   RedundantSmartptrGetCheck.cpp
   RedundantStringCStrCheck.cpp
   RedundantStringInitCheck.cpp
Index: clang-tools-extra/trunk/clang-tidy/readability/ReadabilityTidyModule.cpp
===
--- clang-tools-extra/trunk/clang-tidy/readability/ReadabilityTidyModule.cpp
+++ clang-tools-extra/trunk/clang-tidy/readability/ReadabilityTidyModule.cpp
@@ -31,6 +31,7 @@
 #include "RedundantDeclarationCheck.h"
 #include "RedundantFunctionPtrDereferenceCheck.h"
 #include "RedundantMemberInitCheck.h"
+#include "RedundantPreprocessorCheck.h"
 #include "RedundantSmartptrGetCheck.h"
 #include "RedundantStringCStrCheck.h"
 #include "RedundantStringInitCheck.h"
@@ -83,6 +84,8 @@
 "readability-redundant-function-ptr-dereference");
 CheckFactories.registerCheck(
 "readability-redundant-member-init");
+CheckFactories.registerCheck(
+"readability-redundant-preprocessor");
 CheckFactories.registerCheck(
 "readability-simplify-subscript-expr");
 CheckFactories.registerCheck(
Index: clang-tools-extra/trunk/clang-tidy/readability/RedundantPreprocessorCheck.cpp
===
--- clang-tools-extra/trunk/clang-tidy/readability/RedundantPreprocessorCheck.cpp
+++ clang-tools-extra/trunk/clang-tidy/readability/RedundantPreprocessorCheck.cpp
@@ -0,0 +1,109 @@
+//===--- RedundantPreprocessorCheck.cpp - clang-tidy --===//
+//
+// The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===--===//
+
+#include "RedundantPreprocessorCheck.h"
+#include "clang/Frontend/CompilerInstance.h"
+
+namespace clang {
+namespace tidy {
+namespace readability {
+
+namespace {
+/// Information about an opening preprocessor directive.
+struct PreprocessorEntry {
+  SourceLocation Loc;
+  /// Condition used after the preprocessor directive.
+  std::string Condition;
+};
+
+class RedundantPreprocessorCallbacks : public PPCallbacks {
+  enum DirectiveKind { DK_If = 0, DK_Ifdef = 1, DK_Ifndef = 2 };
+
+public:
+  explicit RedundantPreprocessorCallbacks(ClangTidyCheck ,
+  Preprocessor )
+  : Check(Check), PP(PP),
+WarningDescription("nested redundant %select{#if|#ifdef|#ifndef}0; "
+   "consider removing it"),
+NoteDescription("previous %select{#if|#ifdef|#ifndef}0 was here") {}
+
+  void If(SourceLocation Loc, SourceRange ConditionRange,
+  ConditionValueKind ConditionValue) override {
+StringRef Condition =
+Lexer::getSourceText(CharSourceRange::getTokenRange(ConditionRange),
+ PP.getSourceManager(), PP.getLangOpts());
+CheckMacroRedundancy(Loc, Condition, IfStack, DK_If, DK_If, true);
+  }
+
+  void Ifdef(SourceLocation Loc, const Token ,
+ const MacroDefinition ) override {
+std::string MacroName = PP.getSpelling(MacroNameTok);
+CheckMacroRedundancy(Loc, MacroName, IfdefStack, DK_Ifdef, DK_Ifdef, true);
+CheckMacroRedundancy(Loc, MacroName, IfndefStack, DK_Ifdef, DK_Ifndef,
+ false);
+  }
+
+  void Ifndef(SourceLocation Loc, 

[PATCH] D54349: [clang-tidy] new check 'readability-redundant-preprocessor'

[Miklos Vajna via Phabricator via cfe-commits]

vmiklos added a comment.

In https://reviews.llvm.org/D54349#1301663, @aaron.ballman wrote:

> I think this check is in good shape for the initial commit. Additional 
> functionality can be added incrementally.


OK, thanks. I'll lcommit this once https://reviews.llvm.org/D54450 is committed.


https://reviews.llvm.org/D54349



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

[PATCH] D54349: [clang-tidy] new check 'readability-redundant-preprocessor'

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

I think this check is in good shape for the initial commit. Additional 
functionality can be added incrementally.




Comment at: test/clang-tidy/readability-redundant-preprocessor.cpp:53
+// CHECK-NOTES: [[@LINE+1]]:2: warning: nested redundant #if; consider 
removing it [readability-redundant-preprocessor]
+#if FOO == 3 + 1
+// CHECK-NOTES: [[@LINE-3]]:2: note: previous #if was here

vmiklos wrote:
> aaron.ballman wrote:
> > I didn't describe my test scenario very well -- can you change this line to 
> > FOO == 4 but leave the preceding conditional check as FOO == 3 + 1? The 
> > goal is to test that this isn't a token-by-token check, but uses the 
> > symbolic values instead.
> That doesn't work at the moment, since `PreprocessorEntry::Condition` is just 
> a string, so `3 + 1` won't equal to `4`. I think we discussed this above 
> already, and you said:
> 
> > I think it requires manual work. There's a FIXME in `PPCallbacks::If()` to 
> > pass a list/tree of tokens that would make implementing this reasonable. 
> > I'd say it's fine as a follow-up patch.
> 
> But later you wrote:
> 
> > I agree that it's more complex, but that's why I was asking for it -- I 
> > don't think the design you have currently will extend for these sort of 
> > cases, and I was hoping to cover more utility with the check to hopefully 
> > shake out those forward-looking design considerations. As it stands, I feel 
> > like this check is a bit too simplistic to have much value, but if you 
> > covered some of these other cases, it would alleviate that worry for me.
> 
> My hope is that the check with its current "feature set" already has some 
> value, but let me know if I'm too optimistic. :-)
> 
> That being said, if I want to support simple cases like "realize that `3 + 1` 
> and `4` is the same" -- do you imagine that would be manually handled in this 
> check or there is already some reusable building block in the codebase to 
> evaluate if two expressions have the same integer value? I guess doing this 
> manually is a lot of work, e.g. the answer for `FOO + 4` would be "it 
> depends", so that should not equal to `8`, even if FOO happens to be `4`, etc.
That's a good point -- sorry about the mistake on my suggestion that this test 
should pass, I had a temporary lapse. :-)


https://reviews.llvm.org/D54349



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

[PATCH] D54349: [clang-tidy] new check 'readability-redundant-preprocessor'

[Miklos Vajna via Phabricator via cfe-commits]

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



Comment at: test/clang-tidy/readability-redundant-preprocessor.cpp:53
+// CHECK-NOTES: [[@LINE+1]]:2: warning: nested redundant #if; consider 
removing it [readability-redundant-preprocessor]
+#if FOO == 3 + 1
+// CHECK-NOTES: [[@LINE-3]]:2: note: previous #if was here

aaron.ballman wrote:
> I didn't describe my test scenario very well -- can you change this line to 
> FOO == 4 but leave the preceding conditional check as FOO == 3 + 1? The goal 
> is to test that this isn't a token-by-token check, but uses the symbolic 
> values instead.
That doesn't work at the moment, since `PreprocessorEntry::Condition` is just a 
string, so `3 + 1` won't equal to `4`. I think we discussed this above already, 
and you said:

> I think it requires manual work. There's a FIXME in `PPCallbacks::If()` to 
> pass a list/tree of tokens that would make implementing this reasonable. I'd 
> say it's fine as a follow-up patch.

But later you wrote:

> I agree that it's more complex, but that's why I was asking for it -- I don't 
> think the design you have currently will extend for these sort of cases, and 
> I was hoping to cover more utility with the check to hopefully shake out 
> those forward-looking design considerations. As it stands, I feel like this 
> check is a bit too simplistic to have much value, but if you covered some of 
> these other cases, it would alleviate that worry for me.

My hope is that the check with its current "feature set" already has some 
value, but let me know if I'm too optimistic. :-)

That being said, if I want to support simple cases like "realize that `3 + 1` 
and `4` is the same" -- do you imagine that would be manually handled in this 
check or there is already some reusable building block in the codebase to 
evaluate if two expressions have the same integer value? I guess doing this 
manually is a lot of work, e.g. the answer for `FOO + 4` would be "it depends", 
so that should not equal to `8`, even if FOO happens to be `4`, etc.


https://reviews.llvm.org/D54349



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

[PATCH] D54349: [clang-tidy] new check 'readability-redundant-preprocessor'

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.



Comment at: test/clang-tidy/readability-redundant-preprocessor.cpp:53
+// CHECK-NOTES: [[@LINE+1]]:2: warning: nested redundant #if; consider 
removing it [readability-redundant-preprocessor]
+#if FOO == 3 + 1
+// CHECK-NOTES: [[@LINE-3]]:2: note: previous #if was here

I didn't describe my test scenario very well -- can you change this line to FOO 
== 4 but leave the preceding conditional check as FOO == 3 + 1? The goal is to 
test that this isn't a token-by-token check, but uses the symbolic values 
instead.


https://reviews.llvm.org/D54349



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

[PATCH] D54349: [clang-tidy] new check 'readability-redundant-preprocessor'

[Miklos Vajna via Phabricator via cfe-commits]

vmiklos marked 2 inline comments as done.
vmiklos added inline comments.



Comment at: clang-tidy/readability/RedundantPreprocessorCheck.cpp:37
+CheckMacroRedundancy(Loc, Condition, IfStack,
+ "nested redundant if; consider removing it",
+ "previous if was here", true);

aaron.ballman wrote:
> I think these diagnostics should be hoisted as private constant members of 
> the class. Something like:
> `nested redundant %select{#if|#ifdef|#ifndef}0; consider removing it"` and 
> `previous %select{#if|#ifdef|#ifndef}0 here`
Done, I've also added an enum to specify if/ifdef/ifndef to avoid hard-to-read 
0/1/2 for these.


https://reviews.llvm.org/D54349



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

[PATCH] D54349: [clang-tidy] new check 'readability-redundant-preprocessor'

[Miklos Vajna via Phabricator via cfe-commits]

vmiklos updated this revision to Diff 174439.

https://reviews.llvm.org/D54349

Files:
  clang-tidy/readability/CMakeLists.txt
  clang-tidy/readability/ReadabilityTidyModule.cpp
  clang-tidy/readability/RedundantPreprocessorCheck.cpp
  clang-tidy/readability/RedundantPreprocessorCheck.h
  docs/ReleaseNotes.rst
  docs/clang-tidy/checks/list.rst
  docs/clang-tidy/checks/readability-redundant-preprocessor.rst
  test/clang-tidy/readability-redundant-preprocessor-ifdef.cpp
  test/clang-tidy/readability-redundant-preprocessor.cpp
  test/clang-tidy/readability-redundant-preprocessor.h

Index: test/clang-tidy/readability-redundant-preprocessor.h
===
--- /dev/null
+++ test/clang-tidy/readability-redundant-preprocessor.h
@@ -0,0 +1,5 @@
+#ifndef FOO
+#ifndef FOO // this would warn, but not in a header
+void f();
+#endif
+#endif
Index: test/clang-tidy/readability-redundant-preprocessor.cpp
===
--- /dev/null
+++ test/clang-tidy/readability-redundant-preprocessor.cpp
@@ -0,0 +1,84 @@
+// RUN: %check_clang_tidy %s readability-redundant-preprocessor %t -- -- -I %S
+
+// Positive testing.
+#ifndef FOO
+// CHECK-NOTES: [[@LINE+1]]:2: warning: nested redundant #ifndef; consider removing it [readability-redundant-preprocessor]
+#ifndef FOO
+// CHECK-NOTES: [[@LINE-3]]:2: note: previous #ifndef was here
+void f();
+#endif
+#endif
+
+// Positive testing of inverted condition.
+#ifndef FOO
+// CHECK-NOTES: [[@LINE+1]]:2: warning: nested redundant #ifdef; consider removing it [readability-redundant-preprocessor]
+#ifdef FOO
+// CHECK-NOTES: [[@LINE-3]]:2: note: previous #ifndef was here
+void f2();
+#endif
+#endif
+
+// Negative testing.
+#include "readability-redundant-preprocessor.h"
+
+#ifndef BAR
+void g();
+#endif
+
+#ifndef FOO
+#ifndef BAR
+void h();
+#endif
+#endif
+
+#ifndef FOO
+#ifdef BAR
+void i();
+#endif
+#endif
+
+// Positive #if testing.
+#define FOO 4
+
+#if FOO == 4
+// CHECK-NOTES: [[@LINE+1]]:2: warning: nested redundant #if; consider removing it [readability-redundant-preprocessor]
+#if FOO == 4
+// CHECK-NOTES: [[@LINE-3]]:2: note: previous #if was here
+void j();
+#endif
+#endif
+
+#if FOO == 3 + 1
+// CHECK-NOTES: [[@LINE+1]]:2: warning: nested redundant #if; consider removing it [readability-redundant-preprocessor]
+#if FOO == 3 + 1
+// CHECK-NOTES: [[@LINE-3]]:2: note: previous #if was here
+void j();
+#endif
+#endif
+
+#if FOO == \
+4
+// CHECK-NOTES: [[@LINE+1]]:2: warning: nested redundant #if; consider removing it [readability-redundant-preprocessor]
+#if FOO == \
+4
+// CHECK-NOTES: [[@LINE-5]]:2: note: previous #if was here
+void j();
+#endif
+#endif
+
+// Negative #if testing.
+#define BAR 4
+
+#if FOO == 4
+#if BAR == 4
+void k();
+#endif
+#endif
+
+#if FOO == \
+4
+#if BAR == \
+5
+void k();
+#endif
+#endif
Index: test/clang-tidy/readability-redundant-preprocessor-ifdef.cpp
===
--- /dev/null
+++ test/clang-tidy/readability-redundant-preprocessor-ifdef.cpp
@@ -0,0 +1,36 @@
+// RUN: %check_clang_tidy %s readability-redundant-preprocessor %t -- -- -DFOO
+
+// Positive testing.
+#ifdef FOO
+// CHECK-NOTES: [[@LINE+1]]:2: warning: nested redundant #ifdef; consider removing it [readability-redundant-preprocessor]
+#ifdef FOO
+// CHECK-NOTES: [[@LINE-3]]:2: note: previous #ifdef was here
+void f();
+#endif
+#endif
+
+// Positive testing of inverted condition.
+#ifdef FOO
+// CHECK-NOTES: [[@LINE+1]]:2: warning: nested redundant #ifndef; consider removing it [readability-redundant-preprocessor]
+#ifndef FOO
+// CHECK-NOTES: [[@LINE-3]]:2: note: previous #ifdef was here
+void f2();
+#endif
+#endif
+
+// Negative testing.
+#ifdef BAR
+void g();
+#endif
+
+#ifdef FOO
+#ifdef BAR
+void h();
+#endif
+#endif
+
+#ifdef FOO
+#ifndef BAR
+void i();
+#endif
+#endif
Index: docs/clang-tidy/checks/readability-redundant-preprocessor.rst
===
--- /dev/null
+++ docs/clang-tidy/checks/readability-redundant-preprocessor.rst
@@ -0,0 +1,61 @@
+.. title:: clang-tidy - readability-redundant-preprocessor
+
+readability-redundant-preprocessor
+==
+
+Finds potentially redundant preprocessor directives. At the moment the
+following cases are detected:
+
+* `#ifdef` .. `#endif` pairs which are nested inside an outer pair with the
+  same condition. For example:
+
+.. code-block:: c++
+
+  #ifdef FOO
+  #ifdef FOO // inner ifdef is considered redundant
+  void f();
+  #endif
+  #endif
+
+* Same for `#ifndef` .. `#endif` pairs. For example:
+
+.. code-block:: c++
+
+  #ifndef FOO
+  #ifndef FOO // inner ifndef is considered redundant
+  void f();
+  #endif
+  #endif
+
+* `#ifndef` inside an `#ifdef` with the same condition:
+
+.. code-block:: c++
+
+  #ifdef FOO
+  #ifndef FOO // inner ifndef is considered redundant
+  void f();
+  

[PATCH] D54349: [clang-tidy] new check 'readability-redundant-preprocessor'

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.



Comment at: clang-tidy/readability/RedundantPreprocessorCheck.cpp:37
+CheckMacroRedundancy(Loc, Condition, IfStack,
+ "nested redundant if; consider removing it",
+ "previous if was here", true);

I think these diagnostics should be hoisted as private constant members of the 
class. Something like:
`nested redundant %select{#if|#ifdef|#ifndef}0; consider removing it"` and 
`previous %select{#if|#ifdef|#ifndef}0 here`



Comment at: test/clang-tidy/readability-redundant-preprocessor.cpp:45
+// CHECK-NOTES: [[@LINE+1]]:2: warning: nested redundant if; consider removing 
it [readability-redundant-preprocessor]
+#if FOO == 4
+// CHECK-NOTES: [[@LINE-3]]:2: note: previous if was here

Can you add a test like `#if FOO == 3 + 1` as well?


https://reviews.llvm.org/D54349



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

[PATCH] D54349: [clang-tidy] new check 'readability-redundant-preprocessor'

[Miklos Vajna via Phabricator via cfe-commits]

vmiklos updated this revision to Diff 174021.
vmiklos marked 2 inline comments as done.

https://reviews.llvm.org/D54349

Files:
  clang-tidy/readability/CMakeLists.txt
  clang-tidy/readability/ReadabilityTidyModule.cpp
  clang-tidy/readability/RedundantPreprocessorCheck.cpp
  clang-tidy/readability/RedundantPreprocessorCheck.h
  docs/ReleaseNotes.rst
  docs/clang-tidy/checks/list.rst
  docs/clang-tidy/checks/readability-redundant-preprocessor.rst
  test/clang-tidy/readability-redundant-preprocessor-ifdef.cpp
  test/clang-tidy/readability-redundant-preprocessor.cpp
  test/clang-tidy/readability-redundant-preprocessor.h

Index: test/clang-tidy/readability-redundant-preprocessor.h
===
--- /dev/null
+++ test/clang-tidy/readability-redundant-preprocessor.h
@@ -0,0 +1,5 @@
+#ifndef FOO
+#ifndef FOO // this would warn, but not in a header
+void f();
+#endif
+#endif
Index: test/clang-tidy/readability-redundant-preprocessor.cpp
===
--- /dev/null
+++ test/clang-tidy/readability-redundant-preprocessor.cpp
@@ -0,0 +1,76 @@
+// RUN: %check_clang_tidy %s readability-redundant-preprocessor %t -- -- -I %S
+
+// Positive testing.
+#ifndef FOO
+// CHECK-NOTES: [[@LINE+1]]:2: warning: nested redundant ifndef; consider removing it [readability-redundant-preprocessor]
+#ifndef FOO
+// CHECK-NOTES: [[@LINE-3]]:2: note: previous ifndef was here
+void f();
+#endif
+#endif
+
+// Positive testing of inverted condition.
+#ifndef FOO
+// CHECK-NOTES: [[@LINE+1]]:2: warning: nested redundant ifdef; consider removing it [readability-redundant-preprocessor]
+#ifdef FOO
+// CHECK-NOTES: [[@LINE-3]]:2: note: previous ifndef was here
+void f2();
+#endif
+#endif
+
+// Negative testing.
+#include "readability-redundant-preprocessor.h"
+
+#ifndef BAR
+void g();
+#endif
+
+#ifndef FOO
+#ifndef BAR
+void h();
+#endif
+#endif
+
+#ifndef FOO
+#ifdef BAR
+void i();
+#endif
+#endif
+
+// Positive #if testing.
+#define FOO 4
+
+#if FOO == 4
+// CHECK-NOTES: [[@LINE+1]]:2: warning: nested redundant if; consider removing it [readability-redundant-preprocessor]
+#if FOO == 4
+// CHECK-NOTES: [[@LINE-3]]:2: note: previous if was here
+void j();
+#endif
+#endif
+
+#if FOO == \
+4
+// CHECK-NOTES: [[@LINE+1]]:2: warning: nested redundant if; consider removing it [readability-redundant-preprocessor]
+#if FOO == \
+4
+// CHECK-NOTES: [[@LINE-5]]:2: note: previous if was here
+void j();
+#endif
+#endif
+
+// Negative #if testing.
+#define BAR 4
+
+#if FOO == 4
+#if BAR == 4
+void k();
+#endif
+#endif
+
+#if FOO == \
+4
+#if BAR == \
+5
+void k();
+#endif
+#endif
Index: test/clang-tidy/readability-redundant-preprocessor-ifdef.cpp
===
--- /dev/null
+++ test/clang-tidy/readability-redundant-preprocessor-ifdef.cpp
@@ -0,0 +1,36 @@
+// RUN: %check_clang_tidy %s readability-redundant-preprocessor %t -- -- -DFOO
+
+// Positive testing.
+#ifdef FOO
+// CHECK-NOTES: [[@LINE+1]]:2: warning: nested redundant ifdef; consider removing it [readability-redundant-preprocessor]
+#ifdef FOO
+// CHECK-NOTES: [[@LINE-3]]:2: note: previous ifdef was here
+void f();
+#endif
+#endif
+
+// Positive testing of inverted condition.
+#ifdef FOO
+// CHECK-NOTES: [[@LINE+1]]:2: warning: nested redundant ifndef; consider removing it [readability-redundant-preprocessor]
+#ifndef FOO
+// CHECK-NOTES: [[@LINE-3]]:2: note: previous ifdef was here
+void f2();
+#endif
+#endif
+
+// Negative testing.
+#ifdef BAR
+void g();
+#endif
+
+#ifdef FOO
+#ifdef BAR
+void h();
+#endif
+#endif
+
+#ifdef FOO
+#ifndef BAR
+void i();
+#endif
+#endif
Index: docs/clang-tidy/checks/readability-redundant-preprocessor.rst
===
--- /dev/null
+++ docs/clang-tidy/checks/readability-redundant-preprocessor.rst
@@ -0,0 +1,61 @@
+.. title:: clang-tidy - readability-redundant-preprocessor
+
+readability-redundant-preprocessor
+==
+
+Finds potentially redundant preprocessor directives. At the moment the
+following cases are detected:
+
+* `#ifdef` .. `#endif` pairs which are nested inside an outer pair with the
+  same condition. For example:
+
+.. code-block:: c++
+
+  #ifdef FOO
+  #ifdef FOO // inner ifdef is considered redundant
+  void f();
+  #endif
+  #endif
+
+* Same for `#ifndef` .. `#endif` pairs. For example:
+
+.. code-block:: c++
+
+  #ifndef FOO
+  #ifndef FOO // inner ifndef is considered redundant
+  void f();
+  #endif
+  #endif
+
+* `#ifndef` inside an `#ifdef` with the same condition:
+
+.. code-block:: c++
+
+  #ifdef FOO
+  #ifndef FOO // inner ifndef is considered redundant
+  void f();
+  #endif
+  #endif
+
+* `#ifdef` inside an `#ifndef` with the same condition:
+
+.. code-block:: c++
+
+  #ifndef FOO
+  #ifdef FOO // inner ifdef is considered redundant
+  void f();
+  #endif
+  #endif
+
+* `#if` .. 

[PATCH] D54349: [clang-tidy] new check 'readability-redundant-preprocessor'

[Miklos Vajna via Phabricator via cfe-commits]

vmiklos marked 2 inline comments as done.
vmiklos added inline comments.



Comment at: clang-tidy/readability/ReadabilityTidyModule.cpp:84
+CheckFactories.registerCheck(
+"readability-redundant-preprocessor");
 CheckFactories.registerCheck(

aaron.ballman wrote:
> Please keep this list sorted alphabetically.
Done.



Comment at: clang-tidy/readability/RedundantPreprocessorCheck.cpp:56-59
+StringRef SourceText =
+Lexer::getSourceText(CharSourceRange::getTokenRange(ConditionRange),
+ PP.getSourceManager(), PP.getLangOpts());
+std::string Condition = getCondition(SourceText);

aaron.ballman wrote:
> Szelethus wrote:
> > vmiklos wrote:
> > > Szelethus wrote:
> > > > I'm a little confused. To me, it seems like you acquired the condition 
> > > > already -- doesn't `ConditionRange` actually cover the, well, condition 
> > > > range? This is how I imagined it:
> > > > 
> > > > ```
> > > > #ifdef CUTE_PANDA_CUBS
> > > >^~~
> > > >ConditionRange
> > > > ```
> > > > Why is there a need for `getCondition`? Is there any? If there is 
> > > > (maybe the acquired text contains other things), can you document it? I 
> > > > haven't played with `PPCallbacks` much, so I'm fine with being in the 
> > > > wrong.
> > > ConditionRange covers more than what you expect:
> > > 
> > > ```
> > > #if FOO == 4
> > >^
> > > void f();
> > > ~
> > > #endif
> > > ~~
> > > ```
> > > 
> > > to find out if the condition of the `#if` is the same as a previous one, 
> > > I want to extract just `FOO == 4` from that, then deal with that part 
> > > similar to `#ifdef` and `#ifndef`, which are easier as you have a single 
> > > Token for the condition. But you're right, I should add a comment 
> > > explaining this.
> > Oh my god. There is no tool or a convenient method for this??? I had the 
> > displeasure of working with the preprocessor in the last couple months, and 
> > I get shocked by things like this almost every day.
> > 
> > Yea, unfortunately you will have to write excessive amount of comments to 
> > counterweights the shortcomings of `Preprocessor` :/
> This is working around a bug, and I think it would be better to fix that bug 
> instead of jump through these hoops here.
> 
> `Preprocessor::EvaluateDirectiveExpression()` needs to squirrel away the 
> condition range in the `DirectiveEvalResult` it returns. I'll take a stab at 
> it and report back.
Thanks! I've now rebased this on top of D54450, to be able to drop the ugly 
`getCondition()` function.


https://reviews.llvm.org/D54349



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

[PATCH] D54349: [clang-tidy] new check 'readability-redundant-preprocessor'

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.



Comment at: clang-tidy/readability/ReadabilityTidyModule.cpp:84
+CheckFactories.registerCheck(
+"readability-redundant-preprocessor");
 CheckFactories.registerCheck(

Please keep this list sorted alphabetically.



Comment at: clang-tidy/readability/RedundantPreprocessorCheck.cpp:56-59
+StringRef SourceText =
+Lexer::getSourceText(CharSourceRange::getTokenRange(ConditionRange),
+ PP.getSourceManager(), PP.getLangOpts());
+std::string Condition = getCondition(SourceText);

Szelethus wrote:
> vmiklos wrote:
> > Szelethus wrote:
> > > I'm a little confused. To me, it seems like you acquired the condition 
> > > already -- doesn't `ConditionRange` actually cover the, well, condition 
> > > range? This is how I imagined it:
> > > 
> > > ```
> > > #ifdef CUTE_PANDA_CUBS
> > >^~~
> > >ConditionRange
> > > ```
> > > Why is there a need for `getCondition`? Is there any? If there is (maybe 
> > > the acquired text contains other things), can you document it? I haven't 
> > > played with `PPCallbacks` much, so I'm fine with being in the wrong.
> > ConditionRange covers more than what you expect:
> > 
> > ```
> > #if FOO == 4
> >^
> > void f();
> > ~
> > #endif
> > ~~
> > ```
> > 
> > to find out if the condition of the `#if` is the same as a previous one, I 
> > want to extract just `FOO == 4` from that, then deal with that part similar 
> > to `#ifdef` and `#ifndef`, which are easier as you have a single Token for 
> > the condition. But you're right, I should add a comment explaining this.
> Oh my god. There is no tool or a convenient method for this??? I had the 
> displeasure of working with the preprocessor in the last couple months, and I 
> get shocked by things like this almost every day.
> 
> Yea, unfortunately you will have to write excessive amount of comments to 
> counterweights the shortcomings of `Preprocessor` :/
This is working around a bug, and I think it would be better to fix that bug 
instead of jump through these hoops here.

`Preprocessor::EvaluateDirectiveExpression()` needs to squirrel away the 
condition range in the `DirectiveEvalResult` it returns. I'll take a stab at it 
and report back.


https://reviews.llvm.org/D54349



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

[PATCH] D54349: [clang-tidy] new check 'readability-redundant-preprocessor'

[Umann Kristóf via Phabricator via cfe-commits]

Szelethus added inline comments.



Comment at: clang-tidy/readability/RedundantPreprocessorCheck.cpp:56-59
+StringRef SourceText =
+Lexer::getSourceText(CharSourceRange::getTokenRange(ConditionRange),
+ PP.getSourceManager(), PP.getLangOpts());
+std::string Condition = getCondition(SourceText);

vmiklos wrote:
> Szelethus wrote:
> > I'm a little confused. To me, it seems like you acquired the condition 
> > already -- doesn't `ConditionRange` actually cover the, well, condition 
> > range? This is how I imagined it:
> > 
> > ```
> > #ifdef CUTE_PANDA_CUBS
> >^~~
> >ConditionRange
> > ```
> > Why is there a need for `getCondition`? Is there any? If there is (maybe 
> > the acquired text contains other things), can you document it? I haven't 
> > played with `PPCallbacks` much, so I'm fine with being in the wrong.
> ConditionRange covers more than what you expect:
> 
> ```
> #if FOO == 4
>^
> void f();
> ~
> #endif
> ~~
> ```
> 
> to find out if the condition of the `#if` is the same as a previous one, I 
> want to extract just `FOO == 4` from that, then deal with that part similar 
> to `#ifdef` and `#ifndef`, which are easier as you have a single Token for 
> the condition. But you're right, I should add a comment explaining this.
Oh my god. There is no tool or a convenient method for this??? I had the 
displeasure of working with the preprocessor in the last couple months, and I 
get shocked by things like this almost every day.

Yea, unfortunately you will have to write excessive amount of comments to 
counterweights the shortcomings of `Preprocessor` :/


https://reviews.llvm.org/D54349



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

[PATCH] D54349: [clang-tidy] new check 'readability-redundant-preprocessor'

[Miklos Vajna via Phabricator via cfe-commits]

vmiklos updated this revision to Diff 173575.
vmiklos marked an inline comment as done.

https://reviews.llvm.org/D54349

Files:
  clang-tidy/readability/CMakeLists.txt
  clang-tidy/readability/ReadabilityTidyModule.cpp
  clang-tidy/readability/RedundantPreprocessorCheck.cpp
  clang-tidy/readability/RedundantPreprocessorCheck.h
  docs/ReleaseNotes.rst
  docs/clang-tidy/checks/list.rst
  docs/clang-tidy/checks/readability-redundant-preprocessor.rst
  test/clang-tidy/readability-redundant-preprocessor-ifdef.cpp
  test/clang-tidy/readability-redundant-preprocessor.cpp
  test/clang-tidy/readability-redundant-preprocessor.h

Index: test/clang-tidy/readability-redundant-preprocessor.h
===
--- /dev/null
+++ test/clang-tidy/readability-redundant-preprocessor.h
@@ -0,0 +1,5 @@
+#ifndef FOO
+#ifndef FOO // this would warn, but not in a header
+void f();
+#endif
+#endif
Index: test/clang-tidy/readability-redundant-preprocessor.cpp
===
--- /dev/null
+++ test/clang-tidy/readability-redundant-preprocessor.cpp
@@ -0,0 +1,76 @@
+// RUN: %check_clang_tidy %s readability-redundant-preprocessor %t -- -- -I %S
+
+// Positive testing.
+#ifndef FOO
+// CHECK-NOTES: [[@LINE+1]]:2: warning: nested redundant ifndef; consider removing it [readability-redundant-preprocessor]
+#ifndef FOO
+// CHECK-NOTES: [[@LINE-3]]:2: note: previous ifndef was here
+void f();
+#endif
+#endif
+
+// Positive testing of inverted condition.
+#ifndef FOO
+// CHECK-NOTES: [[@LINE+1]]:2: warning: nested redundant ifdef; consider removing it [readability-redundant-preprocessor]
+#ifdef FOO
+// CHECK-NOTES: [[@LINE-3]]:2: note: previous ifndef was here
+void f2();
+#endif
+#endif
+
+// Negative testing.
+#include "readability-redundant-preprocessor.h"
+
+#ifndef BAR
+void g();
+#endif
+
+#ifndef FOO
+#ifndef BAR
+void h();
+#endif
+#endif
+
+#ifndef FOO
+#ifdef BAR
+void i();
+#endif
+#endif
+
+// Positive #if testing.
+#define FOO 4
+
+#if FOO == 4
+// CHECK-NOTES: [[@LINE+1]]:2: warning: nested redundant if; consider removing it [readability-redundant-preprocessor]
+#if FOO == 4
+// CHECK-NOTES: [[@LINE-3]]:2: note: previous if was here
+void j();
+#endif
+#endif
+
+#if FOO == \
+4
+// CHECK-NOTES: [[@LINE+1]]:2: warning: nested redundant if; consider removing it [readability-redundant-preprocessor]
+#if FOO == \
+4
+// CHECK-NOTES: [[@LINE-5]]:2: note: previous if was here
+void j();
+#endif
+#endif
+
+// Negative #if testing.
+#define BAR 4
+
+#if FOO == 4
+#if BAR == 4
+void k();
+#endif
+#endif
+
+#if FOO == \
+4
+#if BAR == \
+5
+void k();
+#endif
+#endif
Index: test/clang-tidy/readability-redundant-preprocessor-ifdef.cpp
===
--- /dev/null
+++ test/clang-tidy/readability-redundant-preprocessor-ifdef.cpp
@@ -0,0 +1,36 @@
+// RUN: %check_clang_tidy %s readability-redundant-preprocessor %t -- -- -DFOO
+
+// Positive testing.
+#ifdef FOO
+// CHECK-NOTES: [[@LINE+1]]:2: warning: nested redundant ifdef; consider removing it [readability-redundant-preprocessor]
+#ifdef FOO
+// CHECK-NOTES: [[@LINE-3]]:2: note: previous ifdef was here
+void f();
+#endif
+#endif
+
+// Positive testing of inverted condition.
+#ifdef FOO
+// CHECK-NOTES: [[@LINE+1]]:2: warning: nested redundant ifndef; consider removing it [readability-redundant-preprocessor]
+#ifndef FOO
+// CHECK-NOTES: [[@LINE-3]]:2: note: previous ifdef was here
+void f2();
+#endif
+#endif
+
+// Negative testing.
+#ifdef BAR
+void g();
+#endif
+
+#ifdef FOO
+#ifdef BAR
+void h();
+#endif
+#endif
+
+#ifdef FOO
+#ifndef BAR
+void i();
+#endif
+#endif
Index: docs/clang-tidy/checks/readability-redundant-preprocessor.rst
===
--- /dev/null
+++ docs/clang-tidy/checks/readability-redundant-preprocessor.rst
@@ -0,0 +1,61 @@
+.. title:: clang-tidy - readability-redundant-preprocessor
+
+readability-redundant-preprocessor
+==
+
+Finds potentially redundant preprocessor directives. At the moment the
+following cases are detected:
+
+* `#ifdef` .. `#endif` pairs which are nested inside an outer pair with the
+  same condition. For example:
+
+.. code-block:: c++
+
+  #ifdef FOO
+  #ifdef FOO // inner ifdef is considered redundant
+  void f();
+  #endif
+  #endif
+
+* Same for `#ifndef` .. `#endif` pairs. For example:
+
+.. code-block:: c++
+
+  #ifndef FOO
+  #ifndef FOO // inner ifndef is considered redundant
+  void f();
+  #endif
+  #endif
+
+* `#ifndef` inside an `#ifdef` with the same condition:
+
+.. code-block:: c++
+
+  #ifdef FOO
+  #ifndef FOO // inner ifndef is considered redundant
+  void f();
+  #endif
+  #endif
+
+* `#ifdef` inside an `#ifndef` with the same condition:
+
+.. code-block:: c++
+
+  #ifndef FOO
+  #ifdef FOO // inner ifdef is considered redundant
+  void f();
+  #endif
+  #endif
+
+* `#if` .. 

[PATCH] D54349: [clang-tidy] new check 'readability-redundant-preprocessor'

[Miklos Vajna via Phabricator via cfe-commits]

vmiklos marked 2 inline comments as done.
vmiklos added inline comments.



Comment at: clang-tidy/readability/RedundantPreprocessorCheck.cpp:56-59
+StringRef SourceText =
+Lexer::getSourceText(CharSourceRange::getTokenRange(ConditionRange),
+ PP.getSourceManager(), PP.getLangOpts());
+std::string Condition = getCondition(SourceText);

Szelethus wrote:
> I'm a little confused. To me, it seems like you acquired the condition 
> already -- doesn't `ConditionRange` actually cover the, well, condition 
> range? This is how I imagined it:
> 
> ```
> #ifdef CUTE_PANDA_CUBS
>^~~
>ConditionRange
> ```
> Why is there a need for `getCondition`? Is there any? If there is (maybe the 
> acquired text contains other things), can you document it? I haven't played 
> with `PPCallbacks` much, so I'm fine with being in the wrong.
ConditionRange covers more than what you expect:

```
#if FOO == 4
   ^
void f();
~
#endif
~~
```

to find out if the condition of the `#if` is the same as a previous one, I want 
to extract just `FOO == 4` from that, then deal with that part similar to 
`#ifdef` and `#ifndef`, which are easier as you have a single Token for the 
condition. But you're right, I should add a comment explaining this.


https://reviews.llvm.org/D54349



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

[PATCH] D54349: [clang-tidy] new check 'readability-redundant-preprocessor'

[Umann Kristóf via Phabricator via cfe-commits]

Szelethus added inline comments.



Comment at: clang-tidy/readability/RedundantPreprocessorCheck.cpp:56-59
+StringRef SourceText =
+Lexer::getSourceText(CharSourceRange::getTokenRange(ConditionRange),
+ PP.getSourceManager(), PP.getLangOpts());
+std::string Condition = getCondition(SourceText);

I'm a little confused. To me, it seems like you acquired the condition already 
-- doesn't `ConditionRange` actually cover the, well, condition range? This is 
how I imagined it:

```
#ifdef CUTE_PANDA_CUBS
   ^~~
   ConditionRange
```
Why is there a need for `getCondition`? Is there any? If there is (maybe the 
acquired text contains other things), can you document it? I haven't played 
with `PPCallbacks` much, so I'm fine with being in the wrong.


https://reviews.llvm.org/D54349



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

[PATCH] D54349: [clang-tidy] new check 'readability-redundant-preprocessor'

[Miklos Vajna via Phabricator via cfe-commits]

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



Comment at: docs/clang-tidy/checks/readability-redundant-preprocessor.rst:34
+
+ #ifdef FOO
+ #ifndef FOO // inner ifndef is considered redundant

JonasToth wrote:
> The indendation for these examples (following as well) is not enough. Please 
> use 2 spaces.
Fixed.


https://reviews.llvm.org/D54349



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

[PATCH] D54349: [clang-tidy] new check 'readability-redundant-preprocessor'

[Miklos Vajna via Phabricator via cfe-commits]

vmiklos updated this revision to Diff 173572.

https://reviews.llvm.org/D54349

Files:
  clang-tidy/readability/CMakeLists.txt
  clang-tidy/readability/ReadabilityTidyModule.cpp
  clang-tidy/readability/RedundantPreprocessorCheck.cpp
  clang-tidy/readability/RedundantPreprocessorCheck.h
  docs/ReleaseNotes.rst
  docs/clang-tidy/checks/list.rst
  docs/clang-tidy/checks/readability-redundant-preprocessor.rst
  test/clang-tidy/readability-redundant-preprocessor-ifdef.cpp
  test/clang-tidy/readability-redundant-preprocessor.cpp
  test/clang-tidy/readability-redundant-preprocessor.h

Index: test/clang-tidy/readability-redundant-preprocessor.h
===
--- /dev/null
+++ test/clang-tidy/readability-redundant-preprocessor.h
@@ -0,0 +1,5 @@
+#ifndef FOO
+#ifndef FOO // this would warn, but not in a header
+void f();
+#endif
+#endif
Index: test/clang-tidy/readability-redundant-preprocessor.cpp
===
--- /dev/null
+++ test/clang-tidy/readability-redundant-preprocessor.cpp
@@ -0,0 +1,76 @@
+// RUN: %check_clang_tidy %s readability-redundant-preprocessor %t -- -- -I %S
+
+// Positive testing.
+#ifndef FOO
+// CHECK-NOTES: [[@LINE+1]]:2: warning: nested redundant ifndef; consider removing it [readability-redundant-preprocessor]
+#ifndef FOO
+// CHECK-NOTES: [[@LINE-3]]:2: note: previous ifndef was here
+void f();
+#endif
+#endif
+
+// Positive testing of inverted condition.
+#ifndef FOO
+// CHECK-NOTES: [[@LINE+1]]:2: warning: nested redundant ifdef; consider removing it [readability-redundant-preprocessor]
+#ifdef FOO
+// CHECK-NOTES: [[@LINE-3]]:2: note: previous ifndef was here
+void f2();
+#endif
+#endif
+
+// Negative testing.
+#include "readability-redundant-preprocessor.h"
+
+#ifndef BAR
+void g();
+#endif
+
+#ifndef FOO
+#ifndef BAR
+void h();
+#endif
+#endif
+
+#ifndef FOO
+#ifdef BAR
+void i();
+#endif
+#endif
+
+// Positive #if testing.
+#define FOO 4
+
+#if FOO == 4
+// CHECK-NOTES: [[@LINE+1]]:2: warning: nested redundant if; consider removing it [readability-redundant-preprocessor]
+#if FOO == 4
+// CHECK-NOTES: [[@LINE-3]]:2: note: previous if was here
+void j();
+#endif
+#endif
+
+#if FOO == \
+4
+// CHECK-NOTES: [[@LINE+1]]:2: warning: nested redundant if; consider removing it [readability-redundant-preprocessor]
+#if FOO == \
+4
+// CHECK-NOTES: [[@LINE-5]]:2: note: previous if was here
+void j();
+#endif
+#endif
+
+// Negative #if testing.
+#define BAR 4
+
+#if FOO == 4
+#if BAR == 4
+void k();
+#endif
+#endif
+
+#if FOO == \
+4
+#if BAR == \
+5
+void k();
+#endif
+#endif
Index: test/clang-tidy/readability-redundant-preprocessor-ifdef.cpp
===
--- /dev/null
+++ test/clang-tidy/readability-redundant-preprocessor-ifdef.cpp
@@ -0,0 +1,36 @@
+// RUN: %check_clang_tidy %s readability-redundant-preprocessor %t -- -- -DFOO
+
+// Positive testing.
+#ifdef FOO
+// CHECK-NOTES: [[@LINE+1]]:2: warning: nested redundant ifdef; consider removing it [readability-redundant-preprocessor]
+#ifdef FOO
+// CHECK-NOTES: [[@LINE-3]]:2: note: previous ifdef was here
+void f();
+#endif
+#endif
+
+// Positive testing of inverted condition.
+#ifdef FOO
+// CHECK-NOTES: [[@LINE+1]]:2: warning: nested redundant ifndef; consider removing it [readability-redundant-preprocessor]
+#ifndef FOO
+// CHECK-NOTES: [[@LINE-3]]:2: note: previous ifdef was here
+void f2();
+#endif
+#endif
+
+// Negative testing.
+#ifdef BAR
+void g();
+#endif
+
+#ifdef FOO
+#ifdef BAR
+void h();
+#endif
+#endif
+
+#ifdef FOO
+#ifndef BAR
+void i();
+#endif
+#endif
Index: docs/clang-tidy/checks/readability-redundant-preprocessor.rst
===
--- /dev/null
+++ docs/clang-tidy/checks/readability-redundant-preprocessor.rst
@@ -0,0 +1,61 @@
+.. title:: clang-tidy - readability-redundant-preprocessor
+
+readability-redundant-preprocessor
+==
+
+Finds potentially redundant preprocessor directives. At the moment the
+following cases are detected:
+
+* `#ifdef` .. `#endif` pairs which are nested inside an outer pair with the
+  same condition. For example:
+
+.. code-block:: c++
+
+  #ifdef FOO
+  #ifdef FOO // inner ifdef is considered redundant
+  void f();
+  #endif
+  #endif
+
+* Same for `#ifndef` .. `#endif` pairs. For example:
+
+.. code-block:: c++
+
+  #ifndef FOO
+  #ifndef FOO // inner ifndef is considered redundant
+  void f();
+  #endif
+  #endif
+
+* `#ifndef` inside an `#ifdef` with the same condition:
+
+.. code-block:: c++
+
+  #ifdef FOO
+  #ifndef FOO // inner ifndef is considered redundant
+  void f();
+  #endif
+  #endif
+
+* `#ifdef` inside an `#ifndef` with the same condition:
+
+.. code-block:: c++
+
+  #ifndef FOO
+  #ifdef FOO // inner ifdef is considered redundant
+  void f();
+  #endif
+  #endif
+
+* `#if` .. `#endif` pairs which are nested inside an 

[PATCH] D54349: [clang-tidy] new check 'readability-redundant-preprocessor'

[Jonas Toth via Phabricator via cfe-commits]

JonasToth added inline comments.



Comment at: docs/clang-tidy/checks/readability-redundant-preprocessor.rst:34
+
+ #ifdef FOO
+ #ifndef FOO // inner ifndef is considered redundant

The indendation for these examples (following as well) is not enough. Please 
use 2 spaces.


https://reviews.llvm.org/D54349



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

[PATCH] D54349: [clang-tidy] new check 'readability-redundant-preprocessor'

[Miklos Vajna via Phabricator via cfe-commits]

vmiklos added a comment.

In https://reviews.llvm.org/D54349#1294600, @vmiklos wrote:

> Let's see if that works in practice.


I've implemented this now, please take a look. (Extended test + docs as well, 
as usual.) Thanks!


https://reviews.llvm.org/D54349



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

[PATCH] D54349: [clang-tidy] new check 'readability-redundant-preprocessor'

[Miklos Vajna via Phabricator via cfe-commits]

vmiklos updated this revision to Diff 173556.
vmiklos marked an inline comment as done.

https://reviews.llvm.org/D54349

Files:
  clang-tidy/readability/CMakeLists.txt
  clang-tidy/readability/ReadabilityTidyModule.cpp
  clang-tidy/readability/RedundantPreprocessorCheck.cpp
  clang-tidy/readability/RedundantPreprocessorCheck.h
  docs/ReleaseNotes.rst
  docs/clang-tidy/checks/list.rst
  docs/clang-tidy/checks/readability-redundant-preprocessor.rst
  test/clang-tidy/readability-redundant-preprocessor-ifdef.cpp
  test/clang-tidy/readability-redundant-preprocessor.cpp
  test/clang-tidy/readability-redundant-preprocessor.h

Index: test/clang-tidy/readability-redundant-preprocessor.h
===
--- /dev/null
+++ test/clang-tidy/readability-redundant-preprocessor.h
@@ -0,0 +1,5 @@
+#ifndef FOO
+#ifndef FOO // this would warn, but not in a header
+void f();
+#endif
+#endif
Index: test/clang-tidy/readability-redundant-preprocessor.cpp
===
--- /dev/null
+++ test/clang-tidy/readability-redundant-preprocessor.cpp
@@ -0,0 +1,76 @@
+// RUN: %check_clang_tidy %s readability-redundant-preprocessor %t -- -- -I %S
+
+// Positive testing.
+#ifndef FOO
+// CHECK-NOTES: [[@LINE+1]]:2: warning: nested redundant ifndef; consider removing it [readability-redundant-preprocessor]
+#ifndef FOO
+// CHECK-NOTES: [[@LINE-3]]:2: note: previous ifndef was here
+void f();
+#endif
+#endif
+
+// Positive testing of inverted condition.
+#ifndef FOO
+// CHECK-NOTES: [[@LINE+1]]:2: warning: nested redundant ifdef; consider removing it [readability-redundant-preprocessor]
+#ifdef FOO
+// CHECK-NOTES: [[@LINE-3]]:2: note: previous ifndef was here
+void f2();
+#endif
+#endif
+
+// Negative testing.
+#include "readability-redundant-preprocessor.h"
+
+#ifndef BAR
+void g();
+#endif
+
+#ifndef FOO
+#ifndef BAR
+void h();
+#endif
+#endif
+
+#ifndef FOO
+#ifdef BAR
+void i();
+#endif
+#endif
+
+// Positive #if testing.
+#define FOO 4
+
+#if FOO == 4
+// CHECK-NOTES: [[@LINE+1]]:2: warning: nested redundant if; consider removing it [readability-redundant-preprocessor]
+#if FOO == 4
+// CHECK-NOTES: [[@LINE-3]]:2: note: previous if was here
+void j();
+#endif
+#endif
+
+#if FOO == \
+4
+// CHECK-NOTES: [[@LINE+1]]:2: warning: nested redundant if; consider removing it [readability-redundant-preprocessor]
+#if FOO == \
+4
+// CHECK-NOTES: [[@LINE-5]]:2: note: previous if was here
+void j();
+#endif
+#endif
+
+// Negative #if testing.
+#define BAR 4
+
+#if FOO == 4
+#if BAR == 4
+void k();
+#endif
+#endif
+
+#if FOO == \
+4
+#if BAR == \
+5
+void k();
+#endif
+#endif
Index: test/clang-tidy/readability-redundant-preprocessor-ifdef.cpp
===
--- /dev/null
+++ test/clang-tidy/readability-redundant-preprocessor-ifdef.cpp
@@ -0,0 +1,36 @@
+// RUN: %check_clang_tidy %s readability-redundant-preprocessor %t -- -- -DFOO
+
+// Positive testing.
+#ifdef FOO
+// CHECK-NOTES: [[@LINE+1]]:2: warning: nested redundant ifdef; consider removing it [readability-redundant-preprocessor]
+#ifdef FOO
+// CHECK-NOTES: [[@LINE-3]]:2: note: previous ifdef was here
+void f();
+#endif
+#endif
+
+// Positive testing of inverted condition.
+#ifdef FOO
+// CHECK-NOTES: [[@LINE+1]]:2: warning: nested redundant ifndef; consider removing it [readability-redundant-preprocessor]
+#ifndef FOO
+// CHECK-NOTES: [[@LINE-3]]:2: note: previous ifdef was here
+void f2();
+#endif
+#endif
+
+// Negative testing.
+#ifdef BAR
+void g();
+#endif
+
+#ifdef FOO
+#ifdef BAR
+void h();
+#endif
+#endif
+
+#ifdef FOO
+#ifndef BAR
+void i();
+#endif
+#endif
Index: docs/clang-tidy/checks/readability-redundant-preprocessor.rst
===
--- /dev/null
+++ docs/clang-tidy/checks/readability-redundant-preprocessor.rst
@@ -0,0 +1,61 @@
+.. title:: clang-tidy - readability-redundant-preprocessor
+
+readability-redundant-preprocessor
+==
+
+Finds potentially redundant preprocessor directives. At the moment the
+following cases are detected:
+
+* `#ifdef` .. `#endif` pairs which are nested inside an outer pair with the
+  same condition. For example:
+
+.. code-block:: c++
+
+  #ifdef FOO
+  #ifdef FOO // inner ifdef is considered redundant
+  void f();
+  #endif
+  #endif
+
+* Same for `#ifndef` .. `#endif` pairs. For example:
+
+.. code-block:: c++
+
+  #ifndef FOO
+  #ifndef FOO // inner ifndef is considered redundant
+  void f();
+  #endif
+  #endif
+
+* `#ifndef` inside an `#ifdef` with the same condition:
+
+.. code-block:: c++
+
+ #ifdef FOO
+ #ifndef FOO // inner ifndef is considered redundant
+ void f();
+ #endif
+ #endif
+
+* `#ifdef` inside an `#ifndef` with the same condition:
+
+.. code-block:: c++
+
+ #ifndef FOO
+ #ifdef FOO // inner ifdef is considered redundant
+ void f();
+ #endif
+ #endif
+
+* `#if` .. `#endif` 

[PATCH] D54349: [clang-tidy] new check 'readability-redundant-preprocessor'

[Miklos Vajna via Phabricator via cfe-commits]

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

> As it stands, I feel like this check is a bit too simplistic to have much 
> value, but if you covered some of these other cases, it would alleviate that 
> worry for me.

I've now added support for detecting inverted conditions when it comes to 
`#ifdef` and `#ifndef` (see extended documentation and testcases). I'll also 
check what can I do for `#if`. I think it would not be too hard to detect just 
repeated conditions. if I have the full range, then I can look for end of the 
first line that does not end with `\`, and that would be a poor man's way to 
get the `#if` condition. Let's see if that works in practice.




Comment at: clang-tidy/readability/RedundantPreprocessorCheck.cpp:19-22
+struct Entry {
+  SourceLocation Loc;
+  std::string MacroName;
+};

Szelethus wrote:
> vmiklos wrote:
> > Szelethus wrote:
> > > This is a way too general name in my opinion. Either include comments 
> > > that describe it, or rename it (preferably both).
> > Renamed to `PreprocessorCondition`, hope it helps. :-)
> I actually meant it for `Entry`, if you hover your mouse over an inline 
> comment, you can see which lines it applies to. Sorry for the confusing 
> communication :D
Done now. Nah, it's more about I'm not so fluent with phabricator. :-)


https://reviews.llvm.org/D54349



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

[PATCH] D54349: [clang-tidy] new check 'readability-redundant-preprocessor'

[Miklos Vajna via Phabricator via cfe-commits]

vmiklos updated this revision to Diff 173554.

https://reviews.llvm.org/D54349

Files:
  clang-tidy/readability/CMakeLists.txt
  clang-tidy/readability/ReadabilityTidyModule.cpp
  clang-tidy/readability/RedundantPreprocessorCheck.cpp
  clang-tidy/readability/RedundantPreprocessorCheck.h
  docs/ReleaseNotes.rst
  docs/clang-tidy/checks/list.rst
  docs/clang-tidy/checks/readability-redundant-preprocessor.rst
  test/clang-tidy/readability-redundant-preprocessor-ifdef.cpp
  test/clang-tidy/readability-redundant-preprocessor.cpp
  test/clang-tidy/readability-redundant-preprocessor.h

Index: test/clang-tidy/readability-redundant-preprocessor.h
===
--- /dev/null
+++ test/clang-tidy/readability-redundant-preprocessor.h
@@ -0,0 +1,5 @@
+#ifndef FOO
+#ifndef FOO // this would warn, but not in a header
+void f();
+#endif
+#endif
Index: test/clang-tidy/readability-redundant-preprocessor.cpp
===
--- /dev/null
+++ test/clang-tidy/readability-redundant-preprocessor.cpp
@@ -0,0 +1,38 @@
+// RUN: %check_clang_tidy %s readability-redundant-preprocessor %t -- -- -I %S
+
+// Positive testing.
+#ifndef FOO
+// CHECK-NOTES: [[@LINE+1]]:2: warning: nested redundant ifndef; consider removing it [readability-redundant-preprocessor]
+#ifndef FOO
+// CHECK-NOTES: [[@LINE-3]]:2: note: previous ifndef was here
+void f();
+#endif
+#endif
+
+// Positive testing of inverted condition.
+#ifndef FOO
+// CHECK-NOTES: [[@LINE+1]]:2: warning: nested redundant ifdef; consider removing it [readability-redundant-preprocessor]
+#ifdef FOO
+// CHECK-NOTES: [[@LINE-3]]:2: note: previous ifndef was here
+void f2();
+#endif
+#endif
+
+// Negative testing.
+#include "readability-redundant-preprocessor.h"
+
+#ifndef BAR
+void g();
+#endif
+
+#ifndef FOO
+#ifndef BAR
+void h();
+#endif
+#endif
+
+#ifndef FOO
+#ifdef BAR
+void i();
+#endif
+#endif
Index: test/clang-tidy/readability-redundant-preprocessor-ifdef.cpp
===
--- /dev/null
+++ test/clang-tidy/readability-redundant-preprocessor-ifdef.cpp
@@ -0,0 +1,36 @@
+// RUN: %check_clang_tidy %s readability-redundant-preprocessor %t -- -- -DFOO
+
+// Positive testing.
+#ifdef FOO
+// CHECK-NOTES: [[@LINE+1]]:2: warning: nested redundant ifdef; consider removing it [readability-redundant-preprocessor]
+#ifdef FOO
+// CHECK-NOTES: [[@LINE-3]]:2: note: previous ifdef was here
+void f();
+#endif
+#endif
+
+// Positive testing of inverted condition.
+#ifdef FOO
+// CHECK-NOTES: [[@LINE+1]]:2: warning: nested redundant ifndef; consider removing it [readability-redundant-preprocessor]
+#ifndef FOO
+// CHECK-NOTES: [[@LINE-3]]:2: note: previous ifdef was here
+void f2();
+#endif
+#endif
+
+// Negative testing.
+#ifdef BAR
+void g();
+#endif
+
+#ifdef FOO
+#ifdef BAR
+void h();
+#endif
+#endif
+
+#ifdef FOO
+#ifndef BAR
+void i();
+#endif
+#endif
Index: docs/clang-tidy/checks/readability-redundant-preprocessor.rst
===
--- /dev/null
+++ docs/clang-tidy/checks/readability-redundant-preprocessor.rst
@@ -0,0 +1,49 @@
+.. title:: clang-tidy - readability-redundant-preprocessor
+
+readability-redundant-preprocessor
+==
+
+Finds potentially redundant preprocessor directives. At the moment the
+following cases are detected:
+
+* `#ifdef` .. `#endif` pairs which are nested inside an outer pair with the
+  same condition. For example:
+
+.. code-block:: c++
+
+  #ifdef FOO
+  #ifdef FOO // inner ifdef is considered redundant
+  void f();
+  #endif
+  #endif
+
+* Same for `#ifndef` .. `#endif` pairs. For example:
+
+.. code-block:: c++
+
+  #ifndef FOO
+  #ifndef FOO // inner ifndef is considered redundant
+  void f();
+  #endif
+  #endif
+
+* `#ifndef` inside an `#ifdef` with the same condition:
+
+.. code-block:: c++
+
+ #ifdef FOO
+ #ifndef FOO // inner ifndef is considered redundant
+ void f();
+ #endif
+ #endif
+
+* `#ifdef` inside an `#ifndef` with the same condition:
+
+.. code-block:: c++
+
+ #ifndef FOO
+ #ifdef FOO
+ void f();
+ #endif
+ #endif
+
Index: docs/clang-tidy/checks/list.rst
===
--- docs/clang-tidy/checks/list.rst
+++ docs/clang-tidy/checks/list.rst
@@ -244,6 +244,7 @@
readability-redundant-declaration
readability-redundant-function-ptr-dereference
readability-redundant-member-init
+   readability-redundant-preprocessor
readability-redundant-smartptr-get
readability-redundant-string-cstr
readability-redundant-string-init
Index: docs/ReleaseNotes.rst
===
--- docs/ReleaseNotes.rst
+++ docs/ReleaseNotes.rst
@@ -154,6 +154,11 @@
   Detects usage of magic numbers, numbers that are used as literals instead of
   introduced via constants or symbols.
 
+- New 

[PATCH] D54349: [clang-tidy] new check 'readability-redundant-preprocessor'

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

In https://reviews.llvm.org/D54349#1294325, @vmiklos wrote:

> > What do you think about code like:
> > 
> >   #if FOO == 4
> >   #if FOO == 4
> >   #endif
> >   #endif
> >   
> >   #if defined(FOO)
> >   #if defined(FOO)
> >   #endif
> >   #endif
> >   
> >   #if !defined(FOO)
> >   #if !defined(FOO)
> >   #endif
> >   #endif
>
> I looked at supporting these, but it's not clear to me how to get e.g. the 
> 'FOO == 4' string (the condition) from the `If()` callback. So far I only see 
> how to get the start location of the condition and the whole range till the 
> end of endif. Do you have a code pointer how to do this, please? (Or OK if I 
> leave this for a follow-up patch?)


I think it requires manual work. There's a FIXME in `PPCallbacks::If()` to pass 
a list/tree of tokens that would make implementing this reasonable. I'd say 
it's fine as a follow-up patch.

>>   #if defined(FOO)
>>   #if !defined(FOO)
>>   #endif
>>   #endif
>>   
>>   #if !defined(FOO)
>>   #if defined(FOO)
>>   #endif
>>   #endif
> 
> I expect handling these correctly is more complex -- I would prefer focusing 
> on matching conditons in this patch, and get back to inverted conditions in a 
> follow-up patch. Is that OK? If we handle inverted conditions, then handling 
> `a && b` and `!a || !b` would make sense as well for example.

I agree that it's more complex, but that's why I was asking for it -- I don't 
think the design you have currently will extend for these sort of cases, and I 
was hoping to cover more utility with the check to hopefully shake out those 
forward-looking design considerations. As it stands, I feel like this check is 
a bit too simplistic to have much value, but if you covered some of these other 
cases, it would alleviate that worry for me.


https://reviews.llvm.org/D54349



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

[PATCH] D54349: [clang-tidy] new check 'readability-redundant-preprocessor'

[Umann Kristóf via Phabricator via cfe-commits]

Szelethus added inline comments.



Comment at: clang-tidy/readability/RedundantPreprocessorCheck.cpp:19-22
+struct Entry {
+  SourceLocation Loc;
+  std::string MacroName;
+};

vmiklos wrote:
> Szelethus wrote:
> > This is a way too general name in my opinion. Either include comments that 
> > describe it, or rename it (preferably both).
> Renamed to `PreprocessorCondition`, hope it helps. :-)
I actually meant it for `Entry`, if you hover your mouse over an inline 
comment, you can see which lines it applies to. Sorry for the confusing 
communication :D


https://reviews.llvm.org/D54349



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

[PATCH] D54349: [clang-tidy] new check 'readability-redundant-preprocessor'

[Miklos Vajna via Phabricator via cfe-commits]

vmiklos updated this revision to Diff 173526.
vmiklos marked an inline comment as done.

https://reviews.llvm.org/D54349

Files:
  clang-tidy/readability/CMakeLists.txt
  clang-tidy/readability/ReadabilityTidyModule.cpp
  clang-tidy/readability/RedundantPreprocessorCheck.cpp
  clang-tidy/readability/RedundantPreprocessorCheck.h
  docs/ReleaseNotes.rst
  docs/clang-tidy/checks/list.rst
  docs/clang-tidy/checks/readability-redundant-preprocessor.rst
  test/clang-tidy/readability-redundant-preprocessor-ifdef.cpp
  test/clang-tidy/readability-redundant-preprocessor.cpp
  test/clang-tidy/readability-redundant-preprocessor.h

Index: test/clang-tidy/readability-redundant-preprocessor.h
===
--- /dev/null
+++ test/clang-tidy/readability-redundant-preprocessor.h
@@ -0,0 +1,5 @@
+#ifndef FOO
+#ifndef FOO // this would warn, but not in a header
+void f();
+#endif
+#endif
Index: test/clang-tidy/readability-redundant-preprocessor.cpp
===
--- /dev/null
+++ test/clang-tidy/readability-redundant-preprocessor.cpp
@@ -0,0 +1,23 @@
+// RUN: %check_clang_tidy %s readability-redundant-preprocessor %t -- -- -I %S
+
+// Positive testing.
+#ifndef FOO
+// CHECK-NOTES: [[@LINE+1]]:2: warning: nested redundant ifndef; consider removing it [readability-redundant-preprocessor]
+#ifndef FOO
+// CHECK-NOTES: [[@LINE-3]]:2: note: previous ifndef was here
+void f();
+#endif
+#endif
+
+// Negative testing.
+#include "readability-redundant-preprocessor.h"
+
+#ifndef BAR
+void g();
+#endif
+
+#ifndef FOO
+#ifndef BAR
+void h();
+#endif
+#endif
Index: test/clang-tidy/readability-redundant-preprocessor-ifdef.cpp
===
--- /dev/null
+++ test/clang-tidy/readability-redundant-preprocessor-ifdef.cpp
@@ -0,0 +1,21 @@
+// RUN: %check_clang_tidy %s readability-redundant-preprocessor %t -- -- -DFOO
+
+// Positive testing.
+#ifdef FOO
+// CHECK-NOTES: [[@LINE+1]]:2: warning: nested redundant ifdef; consider removing it [readability-redundant-preprocessor]
+#ifdef FOO
+// CHECK-NOTES: [[@LINE-3]]:2: note: previous ifdef was here
+void f();
+#endif
+#endif
+
+// Negative testing.
+#ifdef BAR
+void g();
+#endif
+
+#ifdef FOO
+#ifdef BAR
+void h();
+#endif
+#endif
Index: docs/clang-tidy/checks/readability-redundant-preprocessor.rst
===
--- /dev/null
+++ docs/clang-tidy/checks/readability-redundant-preprocessor.rst
@@ -0,0 +1,29 @@
+.. title:: clang-tidy - readability-redundant-preprocessor
+
+readability-redundant-preprocessor
+==
+
+Finds potentially redundant preprocessor directives. At the moment the
+following cases are detected:
+
+* `#ifdef` .. `#endif` pairs which are nested inside an outer pair with the
+  same condition. For example:
+
+.. code-block:: c++
+
+  #ifdef FOO
+  #ifdef FOO // inner ifdef is considered redundant
+  void f();
+  #endif
+  #endif
+
+* Same for `#ifndef` .. `#endif` pairs. For example:
+
+.. code-block:: c++
+
+  #ifndef FOO
+  #ifndef FOO // inner ifndef is considered redundant
+  void f();
+  #endif
+  #endif
+
Index: docs/clang-tidy/checks/list.rst
===
--- docs/clang-tidy/checks/list.rst
+++ docs/clang-tidy/checks/list.rst
@@ -244,6 +244,7 @@
readability-redundant-declaration
readability-redundant-function-ptr-dereference
readability-redundant-member-init
+   readability-redundant-preprocessor
readability-redundant-smartptr-get
readability-redundant-string-cstr
readability-redundant-string-init
Index: docs/ReleaseNotes.rst
===
--- docs/ReleaseNotes.rst
+++ docs/ReleaseNotes.rst
@@ -154,6 +154,11 @@
   Detects usage of magic numbers, numbers that are used as literals instead of
   introduced via constants or symbols.
 
+- New :doc:`readability-redundant-preprocessor
+  ` check.
+
+  Finds potentially redundant preprocessor directives.
+
 - New :doc:`readability-uppercase-literal-suffix
   ` check.
 
Index: clang-tidy/readability/RedundantPreprocessorCheck.h
===
--- /dev/null
+++ clang-tidy/readability/RedundantPreprocessorCheck.h
@@ -0,0 +1,35 @@
+//===--- RedundantPreprocessorCheck.h - clang-tidy --*- C++ -*-===//
+//
+// The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===--===//
+
+#ifndef LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_READABILITY_REDUNDANTPREPROCESSORCHECK_H
+#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_READABILITY_REDUNDANTPREPROCESSORCHECK_H
+
+#include "../ClangTidy.h"
+
+namespace clang {
+namespace tidy {

[PATCH] D54349: [clang-tidy] new check 'readability-redundant-preprocessor'

[Miklos Vajna via Phabricator via cfe-commits]

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



Comment at: clang-tidy/readability/RedundantPreprocessorCheck.cpp:19-22
+struct Entry {
+  SourceLocation Loc;
+  std::string MacroName;
+};

Szelethus wrote:
> This is a way too general name in my opinion. Either include comments that 
> describe it, or rename it (preferably both).
Renamed to `PreprocessorCondition`, hope it helps. :-)


https://reviews.llvm.org/D54349



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

[PATCH] D54349: [clang-tidy] new check 'readability-redundant-preprocessor'

[Miklos Vajna via Phabricator via cfe-commits]

vmiklos added a comment.

> What do you think about code like:
> 
>   #if FOO == 4
>   #if FOO == 4
>   #endif
>   #endif
>   
>   #if defined(FOO)
>   #if defined(FOO)
>   #endif
>   #endif
>   
>   #if !defined(FOO)
>   #if !defined(FOO)
>   #endif
>   #endif

I looked at supporting these, but it's not clear to me how to get e.g. the 'FOO

4' string (the condition) from the `If()` callback. So far I only see how to


get the start location of the condition and the whole range till the end of
endif. Do you have a code pointer how to do this, please? (Or OK if I leave
this for a follow-up patch?)

>   #if defined(FOO)
>   #if !defined(FOO)
>   #endif
>   #endif
>   
>   #if !defined(FOO)
>   #if defined(FOO)
>   #endif
>   #endif

I expect handling these correctly is more complex -- I would prefer focusing on
matching conditons in this patch, and get back to inverted conditions in a
follow-up patch. Is that OK? If we handle inverted conditions, then handling `a
&& b` and `!a || !b` would make sense as well for example.

> Please keep this list sorted alphabetically.

Done.

> This name is not particularly descriptive. This seems to be more like
>  `CheckMacroRedundancy` or something like that?

Makes sense, done.

> This comment should be re-flowed to fit the column width.

Done.

> What constitutes "redundancy"? A bit more exposition here would be useful.

Hopefully "nested directives with the same condition" makes it easier to
understand the intention and current scope.


https://reviews.llvm.org/D54349



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

[PATCH] D54349: [clang-tidy] new check 'readability-redundant-preprocessor'

[Miklos Vajna via Phabricator via cfe-commits]

vmiklos updated this revision to Diff 173524.

https://reviews.llvm.org/D54349

Files:
  clang-tidy/readability/CMakeLists.txt
  clang-tidy/readability/ReadabilityTidyModule.cpp
  clang-tidy/readability/RedundantPreprocessorCheck.cpp
  clang-tidy/readability/RedundantPreprocessorCheck.h
  docs/ReleaseNotes.rst
  docs/clang-tidy/checks/list.rst
  docs/clang-tidy/checks/readability-redundant-preprocessor.rst
  test/clang-tidy/readability-redundant-preprocessor-ifdef.cpp
  test/clang-tidy/readability-redundant-preprocessor.cpp
  test/clang-tidy/readability-redundant-preprocessor.h

Index: test/clang-tidy/readability-redundant-preprocessor.h
===
--- /dev/null
+++ test/clang-tidy/readability-redundant-preprocessor.h
@@ -0,0 +1,5 @@
+#ifndef FOO
+#ifndef FOO // this would warn, but not in a header
+void f();
+#endif
+#endif
Index: test/clang-tidy/readability-redundant-preprocessor.cpp
===
--- /dev/null
+++ test/clang-tidy/readability-redundant-preprocessor.cpp
@@ -0,0 +1,23 @@
+// RUN: %check_clang_tidy %s readability-redundant-preprocessor %t -- -- -I %S
+
+// Positive testing.
+#ifndef FOO
+// CHECK-NOTES: [[@LINE+1]]:2: warning: nested redundant ifndef; consider removing it [readability-redundant-preprocessor]
+#ifndef FOO
+// CHECK-NOTES: [[@LINE-3]]:2: note: previous ifndef was here
+void f();
+#endif
+#endif
+
+// Negative testing.
+#include "readability-redundant-preprocessor.h"
+
+#ifndef BAR
+void g();
+#endif
+
+#ifndef FOO
+#ifndef BAR
+void h();
+#endif
+#endif
Index: test/clang-tidy/readability-redundant-preprocessor-ifdef.cpp
===
--- /dev/null
+++ test/clang-tidy/readability-redundant-preprocessor-ifdef.cpp
@@ -0,0 +1,21 @@
+// RUN: %check_clang_tidy %s readability-redundant-preprocessor %t -- -- -DFOO
+
+// Positive testing.
+#ifdef FOO
+// CHECK-NOTES: [[@LINE+1]]:2: warning: nested redundant ifdef; consider removing it [readability-redundant-preprocessor]
+#ifdef FOO
+// CHECK-NOTES: [[@LINE-3]]:2: note: previous ifdef was here
+void f();
+#endif
+#endif
+
+// Negative testing.
+#ifdef BAR
+void g();
+#endif
+
+#ifdef FOO
+#ifdef BAR
+void h();
+#endif
+#endif
Index: docs/clang-tidy/checks/readability-redundant-preprocessor.rst
===
--- /dev/null
+++ docs/clang-tidy/checks/readability-redundant-preprocessor.rst
@@ -0,0 +1,29 @@
+.. title:: clang-tidy - readability-redundant-preprocessor
+
+readability-redundant-preprocessor
+==
+
+Finds potentially redundant preprocessor directives. At the moment the
+following cases are detected:
+
+* `#ifdef` .. `#endif` pairs which are nested inside an outer pair with the
+  same condition. For example:
+
+.. code-block:: c++
+
+  #ifdef FOO
+  #ifdef FOO // inner ifdef is considered redundant
+  void f();
+  #endif
+  #endif
+
+* Same for `#ifndef` .. `#endif` pairs. For example:
+
+.. code-block:: c++
+
+  #ifndef FOO
+  #ifndef FOO // inner ifndef is considered redundant
+  void f();
+  #endif
+  #endif
+
Index: docs/clang-tidy/checks/list.rst
===
--- docs/clang-tidy/checks/list.rst
+++ docs/clang-tidy/checks/list.rst
@@ -244,6 +244,7 @@
readability-redundant-declaration
readability-redundant-function-ptr-dereference
readability-redundant-member-init
+   readability-redundant-preprocessor
readability-redundant-smartptr-get
readability-redundant-string-cstr
readability-redundant-string-init
Index: docs/ReleaseNotes.rst
===
--- docs/ReleaseNotes.rst
+++ docs/ReleaseNotes.rst
@@ -154,6 +154,11 @@
   Detects usage of magic numbers, numbers that are used as literals instead of
   introduced via constants or symbols.
 
+- New :doc:`readability-redundant-preprocessor
+  ` check.
+
+  Finds potentially redundant preprocessor directives.
+
 - New :doc:`readability-uppercase-literal-suffix
   ` check.
 
Index: clang-tidy/readability/RedundantPreprocessorCheck.h
===
--- /dev/null
+++ clang-tidy/readability/RedundantPreprocessorCheck.h
@@ -0,0 +1,35 @@
+//===--- RedundantPreprocessorCheck.h - clang-tidy --*- C++ -*-===//
+//
+// The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===--===//
+
+#ifndef LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_READABILITY_REDUNDANTPREPROCESSORCHECK_H
+#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_READABILITY_REDUNDANTPREPROCESSORCHECK_H
+
+#include "../ClangTidy.h"
+
+namespace clang {
+namespace tidy {
+namespace readability {
+
+/// This check 

[PATCH] D54349: [clang-tidy] new check 'readability-redundant-preprocessor'

[Umann Kristóf via Phabricator via cfe-commits]

Szelethus added inline comments.



Comment at: clang-tidy/readability/RedundantPreprocessorCheck.cpp:19-22
+struct Entry {
+  SourceLocation Loc;
+  std::string MacroName;
+};

This is a way too general name in my opinion. Either include comments that 
describe it, or rename it (preferably both).


https://reviews.llvm.org/D54349



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

[PATCH] D54349: [clang-tidy] new check 'readability-redundant-preprocessor'

[Jonas Toth via Phabricator via cfe-commits]

JonasToth added a comment.

> Done.

Please also mark the inline comments as done. Otherwise its hard to follow if 
there are outstanding issues somewhere, especially if code moves around.


https://reviews.llvm.org/D54349



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

[PATCH] D54349: [clang-tidy] new check 'readability-redundant-preprocessor'

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

What do you think about code like:

  #if FOO == 4
  #if FOO == 4
  #endif
  #endif
  
  #if defined(FOO)
  #if defined(FOO)
  #endif
  #endif
  
  #if !defined(FOO)
  #if !defined(FOO)
  #endif
  #endif
  
  #if defined(FOO)
  #if !defined(FOO)
  #endif
  #endif
  
  #if !defined(FOO)
  #if defined(FOO)
  #endif
  #endif




Comment at: clang-tidy/readability/CMakeLists.txt:23
   ReadabilityTidyModule.cpp
+  RedundantPreprocessorCheck.cpp
   RedundantControlFlowCheck.cpp

Please keep this list sorted alphabetically.



Comment at: clang-tidy/readability/RedundantPreprocessorCheck.cpp:52
+private:
+  void Startif(SourceLocation Loc, const Token ,
+   SmallVector , StringRef Warning,

This name is not particularly descriptive. This seems to be more like 
`CheckMacroRedundancy` or something like that?



Comment at: clang-tidy/readability/RedundantPreprocessorCheck.h:1-2
+//===--- RedundantPreprocessorCheck.h - clang-tidy ---*- 
C++
+//-*-===//
+//

This comment should be re-flowed to fit the column width.



Comment at: clang-tidy/readability/RedundantPreprocessorCheck.h:20
+
+/// This check flags redundant preprocessor usage.
+///

What constitutes "redundancy"? A bit more exposition here would be useful.


https://reviews.llvm.org/D54349



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

[PATCH] D54349: [clang-tidy] new check 'readability-redundant-preprocessor'

[Miklos Vajna via Phabricator via cfe-commits]

vmiklos updated this revision to Diff 173468.
vmiklos edited the summary of this revision.

https://reviews.llvm.org/D54349

Files:
  clang-tidy/readability/CMakeLists.txt
  clang-tidy/readability/ReadabilityTidyModule.cpp
  clang-tidy/readability/RedundantPreprocessorCheck.cpp
  clang-tidy/readability/RedundantPreprocessorCheck.h
  docs/ReleaseNotes.rst
  docs/clang-tidy/checks/list.rst
  docs/clang-tidy/checks/readability-redundant-preprocessor.rst
  test/clang-tidy/readability-redundant-preprocessor-ifdef.cpp
  test/clang-tidy/readability-redundant-preprocessor.cpp
  test/clang-tidy/readability-redundant-preprocessor.h

Index: test/clang-tidy/readability-redundant-preprocessor.h
===
--- /dev/null
+++ test/clang-tidy/readability-redundant-preprocessor.h
@@ -0,0 +1,5 @@
+#ifndef FOO
+#ifndef FOO // this would warn, but not in a header
+void f();
+#endif
+#endif
Index: test/clang-tidy/readability-redundant-preprocessor.cpp
===
--- /dev/null
+++ test/clang-tidy/readability-redundant-preprocessor.cpp
@@ -0,0 +1,23 @@
+// RUN: %check_clang_tidy %s readability-redundant-preprocessor %t -- -- -I %S
+
+// Positive testing.
+#ifndef FOO
+// CHECK-NOTES: [[@LINE+1]]:2: warning: nested redundant ifndef; consider removing it [readability-redundant-preprocessor]
+#ifndef FOO
+// CHECK-NOTES: [[@LINE-3]]:2: note: previous ifndef was here
+void f();
+#endif
+#endif
+
+// Negative testing.
+#include "readability-redundant-preprocessor.h"
+
+#ifndef BAR
+void g();
+#endif
+
+#ifndef FOO
+#ifndef BAR
+void h();
+#endif
+#endif
Index: test/clang-tidy/readability-redundant-preprocessor-ifdef.cpp
===
--- /dev/null
+++ test/clang-tidy/readability-redundant-preprocessor-ifdef.cpp
@@ -0,0 +1,21 @@
+// RUN: %check_clang_tidy %s readability-redundant-preprocessor %t -- -- -DFOO
+
+// Positive testing.
+#ifdef FOO
+// CHECK-NOTES: [[@LINE+1]]:2: warning: nested redundant ifdef; consider removing it [readability-redundant-preprocessor]
+#ifdef FOO
+// CHECK-NOTES: [[@LINE-3]]:2: note: previous ifdef was here
+void f();
+#endif
+#endif
+
+// Negative testing.
+#ifdef BAR
+void g();
+#endif
+
+#ifdef FOO
+#ifdef BAR
+void h();
+#endif
+#endif
Index: docs/clang-tidy/checks/readability-redundant-preprocessor.rst
===
--- /dev/null
+++ docs/clang-tidy/checks/readability-redundant-preprocessor.rst
@@ -0,0 +1,29 @@
+.. title:: clang-tidy - readability-redundant-preprocessor
+
+readability-redundant-preprocessor
+==
+
+Finds potentially redundant preprocessor directives. At the moment the
+following cases are detected:
+
+* `#ifdef` .. `#endif` pairs which are nested inside an outer pair with the
+  same condition. For example:
+
+.. code-block:: c++
+
+  #ifdef FOO
+  #ifdef FOO // inner ifdef is considered redundant
+  void f();
+  #endif
+  #endif
+
+* Same for `#ifndef` .. `#endif` pairs. For example:
+
+.. code-block:: c++
+
+  #ifndef FOO
+  #ifndef FOO // inner ifndef is considered redundant
+  void f();
+  #endif
+  #endif
+
Index: docs/clang-tidy/checks/list.rst
===
--- docs/clang-tidy/checks/list.rst
+++ docs/clang-tidy/checks/list.rst
@@ -244,6 +244,7 @@
readability-redundant-declaration
readability-redundant-function-ptr-dereference
readability-redundant-member-init
+   readability-redundant-preprocessor
readability-redundant-smartptr-get
readability-redundant-string-cstr
readability-redundant-string-init
Index: docs/ReleaseNotes.rst
===
--- docs/ReleaseNotes.rst
+++ docs/ReleaseNotes.rst
@@ -154,6 +154,11 @@
   Detects usage of magic numbers, numbers that are used as literals instead of
   introduced via constants or symbols.
 
+- New :doc:`readability-redundant-preprocessor
+  ` check.
+
+  Finds potentially redundant preprocessor directives.
+
 - New :doc:`readability-uppercase-literal-suffix
   ` check.
 
Index: clang-tidy/readability/RedundantPreprocessorCheck.h
===
--- /dev/null
+++ clang-tidy/readability/RedundantPreprocessorCheck.h
@@ -0,0 +1,35 @@
+//===--- RedundantPreprocessorCheck.h - clang-tidy ---*- C++
+//-*-===//
+//
+// The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===--===//
+
+#ifndef LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_READABILITY_REDUNDANTPREPROCESSORCHECK_H
+#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_READABILITY_REDUNDANTPREPROCESSORCHECK_H
+
+#include "../ClangTidy.h"
+
+namespace clang {

[PATCH] D54349: [clang-tidy] new check 'readability-redundant-preprocessor'

[Miklos Vajna via Phabricator via cfe-commits]

vmiklos added a comment.

> preprocessor directives? Same in documentation.

Done.


https://reviews.llvm.org/D54349



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

[PATCH] D54349: [clang-tidy] new check 'readability-redundant-preprocessor'

[Miklos Vajna via Phabricator via cfe-commits]

vmiklos added a comment.

> I think that name is not very descriptive for the user of clang-tidy. `pp`
>  should be `preprocessor` or some other constellation that makes it very clear
>  its about the preprocessor.

Done, renamed to readability-redundant-preprocessor.

> you are in namespace `clang`, you can ellide `clang::`

Done.

> Maybe `SmallVector`? Would be better for performance.

Done.

> I think it would be better to have these methods defined inline, as the
>  splitting does not achieve its original goal (declaration in header,
>  definition in impl).

Done.

> The two functions are copied, please remove this duplicatoin and refactor it
>  to a general parametrized function.

Done.

> Please order the checks alphabetically in the release notes, and one empty
>  line at the end is enough.

Done.

> This needs more explanation, please add `.. code-block:: c++` sections for
>  the cases that demonstrate the issue.

Done.

> Please add a test where the redundancy comes from including other headers. If
>  the headers are nested this case might still occur, but its not safe to
>  diagnose/remove these checks as other include-places might not have the same
>  constellation.
> 
> `ifdef` and `ifndef` are used for the include-guards and therefore
>  particularly necessary to test.

Done. I've added a test to make sure we don't warn in headers, the code for
this was already there, just had no coverage. (Exactly for the reason you
mention, the possibility of include-guards generating false positives.)


https://reviews.llvm.org/D54349



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

[PATCH] D54349: [clang-tidy] new check 'readability-redundant-preprocessor'

[Miklos Vajna via Phabricator via cfe-commits]

vmiklos updated this revision to Diff 173465.
vmiklos retitled this revision from "[clang-tidy] new check 
'readability-redundant-pp'" to "[clang-tidy] new check 
'readability-redundant-preprocessor'".

https://reviews.llvm.org/D54349

Files:
  clang-tidy/readability/CMakeLists.txt
  clang-tidy/readability/ReadabilityTidyModule.cpp
  clang-tidy/readability/RedundantPreprocessorCheck.cpp
  clang-tidy/readability/RedundantPreprocessorCheck.h
  docs/ReleaseNotes.rst
  docs/clang-tidy/checks/list.rst
  docs/clang-tidy/checks/readability-redundant-preprocessor.rst
  test/clang-tidy/readability-redundant-preprocessor-ifdef.cpp
  test/clang-tidy/readability-redundant-preprocessor.cpp
  test/clang-tidy/readability-redundant-preprocessor.h

Index: test/clang-tidy/readability-redundant-preprocessor.h
===
--- /dev/null
+++ test/clang-tidy/readability-redundant-preprocessor.h
@@ -0,0 +1,5 @@
+#ifndef FOO
+#ifndef FOO // this would warn, but not in a header
+void f();
+#endif
+#endif
Index: test/clang-tidy/readability-redundant-preprocessor.cpp
===
--- /dev/null
+++ test/clang-tidy/readability-redundant-preprocessor.cpp
@@ -0,0 +1,23 @@
+// RUN: %check_clang_tidy %s readability-redundant-preprocessor %t -- -- -I %S
+
+// Positive testing.
+#ifndef FOO
+// CHECK-NOTES: [[@LINE+1]]:2: warning: nested redundant ifndef; consider removing it [readability-redundant-preprocessor]
+#ifndef FOO
+// CHECK-NOTES: [[@LINE-3]]:2: note: previous ifndef was here
+void f();
+#endif
+#endif
+
+// Negative testing.
+#include "readability-redundant-preprocessor.h"
+
+#ifndef BAR
+void g();
+#endif
+
+#ifndef FOO
+#ifndef BAR
+void h();
+#endif
+#endif
Index: test/clang-tidy/readability-redundant-preprocessor-ifdef.cpp
===
--- /dev/null
+++ test/clang-tidy/readability-redundant-preprocessor-ifdef.cpp
@@ -0,0 +1,21 @@
+// RUN: %check_clang_tidy %s readability-redundant-preprocessor %t -- -- -DFOO
+
+// Positive testing.
+#ifdef FOO
+// CHECK-NOTES: [[@LINE+1]]:2: warning: nested redundant ifdef; consider removing it [readability-redundant-preprocessor]
+#ifdef FOO
+// CHECK-NOTES: [[@LINE-3]]:2: note: previous ifdef was here
+void f();
+#endif
+#endif
+
+// Negative testing.
+#ifdef BAR
+void g();
+#endif
+
+#ifdef FOO
+#ifdef BAR
+void h();
+#endif
+#endif
Index: docs/clang-tidy/checks/readability-redundant-preprocessor.rst
===
--- /dev/null
+++ docs/clang-tidy/checks/readability-redundant-preprocessor.rst
@@ -0,0 +1,29 @@
+.. title:: clang-tidy - readability-redundant-preprocessor
+
+readability-redundant-preprocessor
+==
+
+Finds potentially redundant preprocessor usage. At the moment the following
+cases are detected:
+
+* `#ifdef` .. `#endif` pairs which are nested inside an outer pair with the
+  same condition. For example:
+
+.. code-block:: c++
+
+  #ifdef FOO
+  #ifdef FOO // inner ifdef is considered redundant
+  void f();
+  #endif
+  #endif
+
+* Same for `#ifndef` .. `#endif` pairs. For example:
+
+.. code-block:: c++
+
+  #ifndef FOO
+  #ifndef FOO // inner ifndef is considered redundant
+  void f();
+  #endif
+  #endif
+
Index: docs/clang-tidy/checks/list.rst
===
--- docs/clang-tidy/checks/list.rst
+++ docs/clang-tidy/checks/list.rst
@@ -244,6 +244,7 @@
readability-redundant-declaration
readability-redundant-function-ptr-dereference
readability-redundant-member-init
+   readability-redundant-preprocessor
readability-redundant-smartptr-get
readability-redundant-string-cstr
readability-redundant-string-init
Index: docs/ReleaseNotes.rst
===
--- docs/ReleaseNotes.rst
+++ docs/ReleaseNotes.rst
@@ -154,6 +154,11 @@
   Detects usage of magic numbers, numbers that are used as literals instead of
   introduced via constants or symbols.
 
+- New :doc:`readability-redundant-preprocessor
+  ` check.
+
+  Finds potentially redundant preprocessor usage.
+
 - New :doc:`readability-uppercase-literal-suffix
   ` check.
 
Index: clang-tidy/readability/RedundantPreprocessorCheck.h
===
--- /dev/null
+++ clang-tidy/readability/RedundantPreprocessorCheck.h
@@ -0,0 +1,35 @@
+//===--- RedundantPreprocessorCheck.h - clang-tidy ---*- C++
+//-*-===//
+//
+// The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===--===//
+
+#ifndef LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_READABILITY_REDUNDANTPREPROCESSORCHECK_H
+#define