Analyze include order on a per-file basis.

The include order check would get notified of all include
directives in a depth-first manner.  This created the
possibility of an include directive from a header file
interfering with the sort order of a set of two distinct
blocks from the top level cpp file, if that include directive
was on just the right line.

With this patch we bucket the include directives by the file
in which they appear in and process one bucket at a time,
so that directives from different files do not get mixed
together into the same list.

Reviewed By: alexfh
Differential Revision: https://reviews.llvm.org/D23434

llvm-svn: 278546

diff --git a/clang-tools-extra/clang-tidy/llvm/IncludeOrderCheck.cpp b/clang-tools-extra/clang-tidy/llvm/IncludeOrderCheck.cpp
index e6d5bc3..c277a45 100644 (file)
--- a/clang-tools-extra/clang-tidy/llvm/IncludeOrderCheck.cpp
+++ b/clang-tools-extra/clang-tidy/llvm/IncludeOrderCheck.cpp
@@ -12,6 +12,8 @@
 #include "clang/Lex/PPCallbacks.h"
 #include "clang/Lex/Preprocessor.h"
 
+#include <map>
+
 namespace clang {
 namespace tidy {
 namespace llvm {
@@ -37,7 +39,9 @@ private:
     bool IsAngled;         ///< true if this was an include with angle brackets
     bool IsMainModule;     ///< true if this was the first include in a file
   };
-  std::vector<IncludeDirective> IncludeDirectives;
+
+  typedef std::vector<IncludeDirective> FileIncludes;
+  std::map<clang::FileID, FileIncludes> IncludeDirectives;
   bool LookForMainModule;
 
   ClangTidyCheck &Check;
@@ -80,7 +84,9 @@ void IncludeOrderPPCallbacks::InclusionDirective(
     ID.IsMainModule = true;
     LookForMainModule = false;
   }
-  IncludeDirectives.push_back(std::move(ID));
+
+  // Bucket the include directives by the id of the file they were declared in.
+  IncludeDirectives[SM.getFileID(HashLoc)].push_back(std::move(ID));
 }
 
 void IncludeOrderPPCallbacks::EndOfMainFile() {
@@ -95,69 +101,73 @@ void IncludeOrderPPCallbacks::EndOfMainFile() {
   // FIXME: We should be more careful about sorting below comments as we don't
   // know if the comment refers to the next include or the whole block that
   // follows.
-  std::vector<unsigned> Blocks(1, 0);
-  for (unsigned I = 1, E = IncludeDirectives.size(); I != E; ++I)
-    if (SM.getExpansionLineNumber(IncludeDirectives[I].Loc) !=
-        SM.getExpansionLineNumber(IncludeDirectives[I - 1].Loc) + 1)
-      Blocks.push_back(I);
-  Blocks.push_back(IncludeDirectives.size()); // Sentinel value.
-
-  // Get a vector of indices.
-  std::vector<unsigned> IncludeIndices;
-  for (unsigned I = 0, E = IncludeDirectives.size(); I != E; ++I)
-    IncludeIndices.push_back(I);
-
-  // Sort the includes. We first sort by priority, then lexicographically.
-  for (unsigned BI = 0, BE = Blocks.size() - 1; BI != BE; ++BI)
-    std::sort(IncludeIndices.begin() + Blocks[BI],
-              IncludeIndices.begin() + Blocks[BI + 1],
-              [this](unsigned LHSI, unsigned RHSI) {
-      IncludeDirective &LHS = IncludeDirectives[LHSI];
-      IncludeDirective &RHS = IncludeDirectives[RHSI];
-
-      int PriorityLHS =
-          getPriority(LHS.Filename, LHS.IsAngled, LHS.IsMainModule);
-      int PriorityRHS =
-          getPriority(RHS.Filename, RHS.IsAngled, RHS.IsMainModule);
-
-      return std::tie(PriorityLHS, LHS.Filename) <
-             std::tie(PriorityRHS, RHS.Filename);
-    });
-
-  // Emit a warning for each block and fixits for all changes within that block.
-  for (unsigned BI = 0, BE = Blocks.size() - 1; BI != BE; ++BI) {
-    // Find the first include that's not in the right position.
-    unsigned I, E;
-    for (I = Blocks[BI], E = Blocks[BI + 1]; I != E; ++I)
-      if (IncludeIndices[I] != I)
-        break;
-
-    if (I == E)
-      continue;
-
-    // Emit a warning.
-    auto D = Check.diag(IncludeDirectives[I].Loc,
-                        "#includes are not sorted properly");
-
-    // Emit fix-its for all following includes in this block.
-    for (; I != E; ++I) {
-      if (IncludeIndices[I] == I)
+  for (auto &Bucket : IncludeDirectives) {
+    auto &FileDirectives = Bucket.second;
+    std::vector<unsigned> Blocks(1, 0);
+    for (unsigned I = 1, E = FileDirectives.size(); I != E; ++I)
+      if (SM.getExpansionLineNumber(FileDirectives[I].Loc) !=
+          SM.getExpansionLineNumber(FileDirectives[I - 1].Loc) + 1)
+        Blocks.push_back(I);
+    Blocks.push_back(FileDirectives.size()); // Sentinel value.
+
+    // Get a vector of indices.
+    std::vector<unsigned> IncludeIndices;
+    for (unsigned I = 0, E = FileDirectives.size(); I != E; ++I)
+      IncludeIndices.push_back(I);
+
+    // Sort the includes. We first sort by priority, then lexicographically.
+    for (unsigned BI = 0, BE = Blocks.size() - 1; BI != BE; ++BI)
+      std::sort(IncludeIndices.begin() + Blocks[BI],
+                IncludeIndices.begin() + Blocks[BI + 1],
+                [&FileDirectives](unsigned LHSI, unsigned RHSI) {
+                  IncludeDirective &LHS = FileDirectives[LHSI];
+                  IncludeDirective &RHS = FileDirectives[RHSI];
+
+                  int PriorityLHS =
+                      getPriority(LHS.Filename, LHS.IsAngled, LHS.IsMainModule);
+                  int PriorityRHS =
+                      getPriority(RHS.Filename, RHS.IsAngled, RHS.IsMainModule);
+
+                  return std::tie(PriorityLHS, LHS.Filename) <
+                         std::tie(PriorityRHS, RHS.Filename);
+                });
+
+    // Emit a warning for each block and fixits for all changes within that
+    // block.
+    for (unsigned BI = 0, BE = Blocks.size() - 1; BI != BE; ++BI) {
+      // Find the first include that's not in the right position.
+      unsigned I, E;
+      for (I = Blocks[BI], E = Blocks[BI + 1]; I != E; ++I)
+        if (IncludeIndices[I] != I)
+          break;
+
+      if (I == E)
         continue;
-      const IncludeDirective &CopyFrom = IncludeDirectives[IncludeIndices[I]];
 
-      SourceLocation FromLoc = CopyFrom.Range.getBegin();
-      const char *FromData = SM.getCharacterData(FromLoc);
-      unsigned FromLen = std::strcspn(FromData, "\n");
+      // Emit a warning.
+      auto D = Check.diag(FileDirectives[I].Loc,
+                          "#includes are not sorted properly");
+
+      // Emit fix-its for all following includes in this block.
+      for (; I != E; ++I) {
+        if (IncludeIndices[I] == I)
+          continue;
+        const IncludeDirective &CopyFrom = FileDirectives[IncludeIndices[I]];
+
+        SourceLocation FromLoc = CopyFrom.Range.getBegin();
+        const char *FromData = SM.getCharacterData(FromLoc);
+        unsigned FromLen = std::strcspn(FromData, "\n");
 
-      StringRef FixedName(FromData, FromLen);
+        StringRef FixedName(FromData, FromLen);
 
-      SourceLocation ToLoc = IncludeDirectives[I].Range.getBegin();
-      const char *ToData = SM.getCharacterData(ToLoc);
-      unsigned ToLen = std::strcspn(ToData, "\n");
-      auto ToRange =
-          CharSourceRange::getCharRange(ToLoc, ToLoc.getLocWithOffset(ToLen));
+        SourceLocation ToLoc = FileDirectives[I].Range.getBegin();
+        const char *ToData = SM.getCharacterData(ToLoc);
+        unsigned ToLen = std::strcspn(ToData, "\n");
+        auto ToRange =
+            CharSourceRange::getCharRange(ToLoc, ToLoc.getLocWithOffset(ToLen));
 
-      D << FixItHint::CreateReplacement(ToRange, FixedName);
+        D << FixItHint::CreateReplacement(ToRange, FixedName);
+      }
     }
   }
 

diff --git a/clang-tools-extra/test/clang-tidy/Inputs/Headers/cross-file-a.h b/clang-tools-extra/test/clang-tidy/Inputs/Headers/cross-file-a.h
new file mode 100644 (file)
index 0000000..e69de29

diff --git a/clang-tools-extra/test/clang-tidy/Inputs/Headers/cross-file-b.h b/clang-tools-extra/test/clang-tidy/Inputs/Headers/cross-file-b.h
new file mode 100644 (file)
index 0000000..e69de29

diff --git a/clang-tools-extra/test/clang-tidy/Inputs/Headers/cross-file-c.h b/clang-tools-extra/test/clang-tidy/Inputs/Headers/cross-file-c.h
new file mode 100644 (file)
index 0000000..342dd34
--- /dev/null
+++ b/clang-tools-extra/test/clang-tidy/Inputs/Headers/cross-file-c.h
@@ -0,0 +1,41 @@
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+// The line number of the following include should match the location of the
+// corresponding comment in llvm-include-order.cpp
+#include "cross-file-b.h"

diff --git a/clang-tools-extra/test/clang-tidy/clang-cl-driver.cpp b/clang-tools-extra/test/clang-tidy/clang-cl-driver.cpp
new file mode 100644 (file)
index 0000000..57511f3
--- /dev/null
+++ b/clang-tools-extra/test/clang-tidy/clang-cl-driver.cpp
@@ -0,0 +1,17 @@
+// RUN: clang-tidy -checks=-*,modernize-use-nullptr %s -- clang-cl /DTEST1 /DFOO=foo /DBAR=bar | FileCheck -implicit-check-not="{{warning|error}}:" %s
+int *a = 0;
+// CHECK: :[[@LINE-1]]:10: warning: use nullptr
+#ifdef TEST1
+int *b = 0;
+// CHECK: :[[@LINE-1]]:10: warning: use nullptr
+#endif
+#define foo 1
+#define bar 1
+#if FOO
+int *c = 0;
+// CHECK: :[[@LINE-1]]:10: warning: use nullptr
+#endif
+#if BAR
+int *d = 0;
+// CHECK: :[[@LINE-1]]:10: warning: use nullptr
+#endif
\ No newline at end of file

diff --git a/clang-tools-extra/test/clang-tidy/llvm-include-order.cpp b/clang-tools-extra/test/clang-tidy/llvm-include-order.cpp
index 445beb1..7272353 100644 (file)
--- a/clang-tools-extra/test/clang-tidy/llvm-include-order.cpp
+++ b/clang-tools-extra/test/clang-tidy/llvm-include-order.cpp
@@ -35,3 +35,8 @@
 
 // CHECK-FIXES: #include "a.h"
 // CHECK-FIXES-NEXT: #include "b.h"
+
+// CHECK-MESSAGES-NOT: [[@LINE+1]]:1: warning: #includes are not sorted properly
+#include "cross-file-c.h"
+// This line number should correspond to the position of the #include in cross-file-c.h
+#include "cross-file-a.h"