[PATCH] D7982: Add readability-duplicate-include check to clang-tidy

[Eugene Zelenko via Phabricator via cfe-commits]

Eugene.Zelenko added a comment.

Functionality-wise this check is superseded by Include What You Use 
.




Comment at: 
clang-tools-extra/docs/clang-tidy/checks/readability-duplicate-include.rst:6
+
+Looks for duplicate includes and removes them.  The check maintains a list of
+included files and looks for duplicates.  If a macro is defined or undefined

Please fix double spaces. Same below.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D7982

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

[PATCH] D7982: Add readability-duplicate-include check to clang-tidy

[Richard via Phabricator via cfe-commits]

LegalizeAdulthood updated this revision to Diff 396932.
LegalizeAdulthood added a comment.
Herald added a subscriber: carlosgalvezp.
Herald added a project: clang-tools-extra.

Revive review with updated diff on top-of-tree


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D7982

Files:
  clang-tools-extra/clang-tidy/readability/CMakeLists.txt
  clang-tools-extra/clang-tidy/readability/DuplicateIncludeCheck.cpp
  clang-tools-extra/clang-tidy/readability/DuplicateIncludeCheck.h
  clang-tools-extra/clang-tidy/readability/ReadabilityTidyModule.cpp
  clang-tools-extra/docs/ReleaseNotes.rst
  clang-tools-extra/docs/clang-tidy/checks/list.rst
  clang-tools-extra/docs/clang-tidy/checks/readability-duplicate-include.rst
  clang-tools-extra/test/clang-tidy/checkers/readability-duplicate-include.cpp
  clang-tools-extra/test/clang-tidy/checkers/readability-duplicate-include.h
  clang-tools-extra/test/clang-tidy/checkers/types.h

Index: clang-tools-extra/test/clang-tidy/checkers/types.h
===
--- /dev/null
+++ clang-tools-extra/test/clang-tidy/checkers/types.h
@@ -0,0 +1 @@
+// empty file used by readability-duplicate-include.cpp
Index: clang-tools-extra/test/clang-tidy/checkers/readability-duplicate-include.h
===
--- /dev/null
+++ clang-tools-extra/test/clang-tidy/checkers/readability-duplicate-include.h
@@ -0,0 +1 @@
+// empty file used by readability-duplicate-include.cpp
Index: clang-tools-extra/test/clang-tidy/checkers/readability-duplicate-include.cpp
===
--- /dev/null
+++ clang-tools-extra/test/clang-tidy/checkers/readability-duplicate-include.cpp
@@ -0,0 +1,73 @@
+// RUN: %check_clang_tidy %s readability-duplicate-include %t -- -std=c++11 -I$(dirname %s)
+// REQUIRES: shell
+
+int a;
+#include 
+int b;
+#include 
+int c;
+// CHECK-MESSAGES: :[[@LINE-2]]:1: warning: duplicate include [readability-duplicate-include]
+// CHECK-FIXES:  {{^int a;$}}
+// CHECK-FIXES-NEXT: {{^#include $}}
+// CHECK-FIXES-NEXT: {{^int b;$}}
+// CHECK-FIXES-NEXT: {{^int c;$}}
+
+int d;
+#include 
+int e;
+#include  // extra stuff that will also be removed
+int f;
+// CHECK-MESSAGES: :[[@LINE-2]]:1: warning: {{.*}}
+// CHECK-FIXES:  {{^int d;$}}
+// CHECK-FIXES-NEXT: {{^#include $}}
+// CHECK-FIXES-NEXT: {{^int e;$}}
+// CHECK-FIXES-NEXT: {{^int f;$}}
+
+int g;
+#include "readability-duplicate-include.h"
+int h;
+#include "readability-duplicate-include.h"
+int i;
+// CHECK-MESSAGES: :[[@LINE-2]]:1: warning: {{.*}}
+// CHECK-FIXES:  {{^int g;$}}
+// CHECK-FIXES-NEXT: {{^#include "readability-duplicate-include.h"$}}
+// CHECK-FIXES-NEXT: {{^int h;$}}
+// CHECK-FIXES-NEXT: {{^int i;$}}
+
+#include "types.h"
+
+int j;
+#include 
+int k;
+#include 
+int l;
+// CHECK-MESSAGES: :[[@LINE-2]]:1: warning: {{.*}}
+// CHECK-FIXES:  {{^int j;$}}
+// CHECK-FIXES-NEXT: {{^#include $}}
+// CHECK-FIXES-NEXT: {{^int k;$}}
+// CHECK-FIXES-NEXT: {{^int l;$}}
+
+int m;
+#  include   // lots of space
+int n;
+// CHECK-MESSAGES: :[[@LINE-2]]:9: warning: {{.*}}
+// CHECK-FIXES:  {{^int m;$}}
+// CHECK-FIXES-NEXT: {{^int n;$}}
+
+// defining a macro in the main file resets the included file cache
+#define ARBITRARY_MACRO
+int o;
+#include 
+int p;
+// CHECK-FIXES:  {{^int o;$}}
+// CHECK-FIXES-NEXT: {{^#include $}}
+// CHECK-FIXES-NEXT: {{^int p;$}}
+
+// undefining a macro resets the cache
+#undef ARBITRARY_MACRO
+int q;
+#include 
+int r;
+// CHECK-FIXES:  {{^int q;$}}
+// CHECK-FIXES-NEXT: {{^#include $}}
+// CHECK-FIXES-NEXT: {{^int r;$}}
Index: clang-tools-extra/docs/clang-tidy/checks/readability-duplicate-include.rst
===
--- /dev/null
+++ clang-tools-extra/docs/clang-tidy/checks/readability-duplicate-include.rst
@@ -0,0 +1,48 @@
+.. title:: clang-tidy - readability-duplicate-include
+
+readability-duplicate-include
+=
+
+Looks for duplicate includes and removes them.  The check maintains a list of
+included files and looks for duplicates.  If a macro is defined or undefined
+then the list of included files is cleared.
+
+A common LLVM preprocessor tactic is to define a macro that controls an
+included file and then include it multiple times in order to achieve different
+effects.  Resetting the list of seen includes every time a macro is defined
+or undefined prevents a false positive from that use case.
+
+Examples:
+
+.. code-block:: c++
+
+  #include 
+  #include 
+  #include 
+
+becomes
+
+.. code-block:: c++
+
+  #include 
+  #include 
+
+Because of the intervening macro definitions, this code remains unchanged:
+
+.. code-block:: c++
+
+  class LangOptionsBase {
+  public:
+// Define simple language options (with no accessors).
+  

[PATCH] D7982: Add readability-duplicate-include check to clang-tidy

[Richard via Phabricator via cfe-commits]

LegalizeAdulthood abandoned this revision.
LegalizeAdulthood added a comment.
Herald added subscribers: jdoerfert, mgorny.

Review takes too long to make forward progress; abandoning.


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

https://reviews.llvm.org/D7982



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

Re: [PATCH] D7982: Add readability-duplicate-include check to clang-tidy

[Eugene Zelenko via cfe-commits]

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

Include What You Use detect duplicated include directives. I think will be good 
idea to use it instead of Clang-tidy for much deeper analysis.


http://reviews.llvm.org/D7982



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

Re: [PATCH] D7982: Add readability-duplicate-include check to clang-tidy

[Richard via cfe-commits]

LegalizeAdulthood added inline comments.


Comment at: clang-tidy/readability/DuplicateIncludeCheck.cpp:62
@@ +61,3 @@
+StringRef SearchPath, StringRef RelativePath, const Module *Imported) {
+  if (!SM_.isInMainFile(HashLoc)) {
+return;

LegalizeAdulthood wrote:
> LegalizeAdulthood wrote:
> > alexfh wrote:
> > > LegalizeAdulthood wrote:
> > > > alexfh wrote:
> > > > > What's the reason to limit the check to the main file only? I think, 
> > > > > it should work on all headers as well. Also, sometimes it's fine to 
> > > > > have duplicate includes even without macro definitions in between, 
> > > > > e.g. when these #includes are in different namespaces.
> > > > > 
> > > > > I'd suggest using the same technique as in the IncludeOrderCheck: for 
> > > > > each file collect all preprocessor directives sorted by 
> > > > > SourceLocation. Then detect #include blocks (not necessarily the same 
> > > > > way as its done in the IncludeOrderCheck. Maybe use the presense of 
> > > > > any non-comment tokens between #includes as a boundary of blocks), 
> > > > > and detect duplicate includes in each block.
> > > > If I remove the `isInMainFile`, how do I ensure that I don't attempt to 
> > > > modify system headers?
> > > Using `SourceLocation::isInSystemHeader()`.
> > Thanks, I'll try that.  The next question that brings to mind is how do I 
> > distinguish between headers that I own and headers used from third party 
> > libraries?
> > 
> > For instance, suppose I run a check on a clang refactoring tool and it uses 
> > `isInSystemHeader` and starts flagging issues in the clang tooling library 
> > headers.
> > 
> > The `compile_commands.json` doesn't contain any information about headers 
> > in my project, only translation units in my build, so it doesn't know 
> > whether or not included headers belong to me or third-party libraries.
> For the benefit of others reading this, Alex pointed out to me the 
> `-header-filter` and `-system-headers` options to clang-tidy.  I think this 
> means I don't need any narrowing if `isExpansinInMainFile()` in any of my 
> matchers.  I will do some experimenting to verify that this doesn't introduce 
> regressions.
If I remove the `isInMainFile` and replace it with a check to 
`isInSystemHeader`, then all my tests fail.  I will have to investigate this 
further.


http://reviews.llvm.org/D7982



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

Re: [PATCH] D7982: Add readability-duplicate-include check to clang-tidy

[Richard via cfe-commits]

LegalizeAdulthood added a comment.

I need to update from review comments and upload a new diff.


http://reviews.llvm.org/D7982



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

Re: [PATCH] D7982: Add readability-duplicate-include check to clang-tidy

[Alexander Kornienko via cfe-commits]

alexfh added a comment.

What's the state of this patch?


http://reviews.llvm.org/D7982



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