Don't allow llvm-include-order to intermingle includes from different files.

[zturner]

Summary

See llvm.org/pr28943 for some context.

I'm not sure if this is the right fix, but it seems to pass the test suite for me.

[zturner]

Fixed the test for this. Confirmed that it fails before the patch and succeeds after the patch.

[zturner]

Seems like dropping all non-main-file include directives is wrong because then it won't be able to detect errors in "non user code". Seems like the real fix is to bucket the include directives by file in which they were declared in, and then process each bucket in order. This way we process all include directives, including those in non-user code, but block analysis is done at a per-file level.

[djasper]

I think we should entirely drop this implementation of the check and let it just check #includes with clang-format. clang-format's implementation isn't a strict superset, e.g. won't sort between multiple blocks, but that's intentional for now.

[alexfh]

The patch LG.

In D23434#513533, @djasper wrote:

I think we should entirely drop this implementation of the check and let it just check #includes with clang-format. clang-format's implementation isn't a strict superset, e.g. won't sort between multiple blocks, but that's intentional for now.

Since sorting across blocks is frequently needed to make include order consistent with the LLVM coding standards, it makes sense to keep this check until clang-format is able to sort across blocks. Especially, since Zachary is specifically interested in this functionality.

[djasper]

I think we got confused. We once had tried to write an experimental separate check to comply with Google's style guide. If you want to fiddle around with that, contact me, I can send you pointers. But as I mentioned we moved away from that. And I think it makes more sense to re-create the sort-across-blocks functionality in clang-format and not in clang-tidy.

[alexfh]

In D23434#513839, @djasper wrote:

I think we got confused. We once had tried to write an experimental separate check to comply with Google's style guide. If you want to fiddle around with that, contact me, I can send you pointers. But as I mentioned we moved away from that. And I think it makes more sense to re-create the sort-across-blocks functionality in clang-format and not in clang-tidy.

Yep, we definitely got confused. That experimental check actually implemented cross-block sorting, but this caused a bunch of issues. Which makes me think that proper implementation of cross-block include sorting is challenging be it in clang-format or clang-tidy. Clang-format probably makes it even more complex, since a higher safety of transformations is expected from it.

clang-tools-extra/trunk/clang-tidy/llvm/IncludeOrderCheck.cpp

 //===--- IncludeOrderCheck.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 "IncludeOrderCheck.h"
 #include "clang/Frontend/CompilerInstance.h"
 #include "clang/Lex/PPCallbacks.h"
 #include "clang/Lex/Preprocessor.h"
 
+#include <map>
+
 namespace clang {
 namespace tidy {
 namespace llvm {
 
 namespace {
 class IncludeOrderPPCallbacks : public PPCallbacks {
 public:
   explicit IncludeOrderPPCallbacks(ClangTidyCheck &Check, SourceManager &SM)

 private:
   struct IncludeDirective {
     SourceLocation Loc;    ///< '#' location in the include directive
     CharSourceRange Range; ///< SourceRange for the file name
     std::string Filename;  ///< Filename as a string
     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;
   SourceManager &SM;
 };
 } // namespace
 
 void IncludeOrderCheck::registerPPCallbacks(CompilerInstance &Compiler) {
void IncludeOrderPPCallbacks::InclusionDirective(
     StringRef SearchPath, StringRef RelativePath, const Module *Imported) {
   // We recognize the first include as a special main module header and want
   // to leave it in the top position.
   IncludeDirective ID = {HashLoc, FilenameRange, FileName, IsAngled, false};
   if (LookForMainModule && !IsAngled) {
     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() {
   LookForMainModule = true;
   if (IncludeDirectives.empty())
     return;
 
   // TODO: find duplicated includes.
 
   // Form blocks of includes. We don't want to sort across blocks. This also
   // implicitly makes us never reorder over #defines or #if directives.
   // 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.
+  for (auto &Bucket : IncludeDirectives) {
+    auto &FileDirectives = Bucket.second;
     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)
+    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(IncludeDirectives.size()); // Sentinel value.
+    Blocks.push_back(FileDirectives.size()); // Sentinel value.
 
     // Get a vector of indices.
     std::vector<unsigned> IncludeIndices;
-  for (unsigned I = 0, E = IncludeDirectives.size(); I != E; ++I)
+    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],
-              [this](unsigned LHSI, unsigned RHSI) {
-      IncludeDirective &LHS = IncludeDirectives[LHSI];
-      IncludeDirective &RHS = IncludeDirectives[RHSI];
+                [&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.
+    // 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,
+      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 = IncludeDirectives[IncludeIndices[I]];
+        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);
 
-      SourceLocation ToLoc = IncludeDirectives[I].Range.getBegin();
+        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);
       }
     }
+  }
 
   IncludeDirectives.clear();
 }
 
 } // namespace llvm
 } // namespace tidy
 } // namespace clang

clang-tools-extra/trunk/test/clang-tidy/Inputs/Headers/cross-file-c.h

+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+// 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"

clang-tools-extra/trunk/test/clang-tidy/clang-cl-driver.cpp

+// 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

clang-tools-extra/trunk/test/clang-tidy/llvm-include-order.cpp

 // CHECK-FIXES-NEXT: #endif
 
 // CHECK-MESSAGES: [[@LINE+1]]:1: warning: #includes are not sorted properly
 #include "b.h"
 #include "a.h"
 
 // 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"