[clang-format] Sort whole block of using declarations while partially formatting

[krasimir]

Summary

This patch enables sorting the full block of using declarations when
some line is affected.

[djasper]

lib/Format/UsingDeclarationsSorter.cpp

79

I'd prefer to iterator over UsingDeclarations here and see whether any of the contained lines is Affected. Seems like a much more encapsulated way to do this and prevents you from needing the awkward in-out parameter.

[krasimir]

• Address review comments

[djasper]

LG.

lib/Format/UsingDeclarationsSorter.cpp

while (Tok && Tok->is(tok::identifier)) {
     Tok = Tok->Next;
   }
   if (HasIdentifier && Tok && Tok->isOneOf(tok::semi, tok::comma))
     return Label;
   return "";
 }
 
 void endUsingDeclarationBlock(
-    SmallVectorImpl<UsingDeclaration> *UsingDeclarations,
+    SmallVectorImpl<UsingDeclaration> *UsingDeclarations, bool *BlockAffected,
     const SourceManager &SourceMgr, tooling::Replacements *Fixes) {
+  if (*BlockAffected) {
     SmallVector<UsingDeclaration, 4> SortedUsingDeclarations(
         UsingDeclarations->begin(), UsingDeclarations->end());
     std::stable_sort(SortedUsingDeclarations.begin(),
                      SortedUsingDeclarations.end());
     for (size_t I = 0, E = UsingDeclarations->size(); I < E; ++I) {
       if ((*UsingDeclarations)[I].Line == SortedUsingDeclarations[I].Line)
         continue;
       auto Begin = (*UsingDeclarations)[I].Line->First->Tok.getLocation();
       auto End = (*UsingDeclarations)[I].Line->Last->Tok.getEndLoc();
       auto SortedBegin =
           SortedUsingDeclarations[I].Line->First->Tok.getLocation();
       auto SortedEnd = SortedUsingDeclarations[I].Line->Last->Tok.getEndLoc();
       StringRef Text(SourceMgr.getCharacterData(SortedBegin),
                      SourceMgr.getCharacterData(SortedEnd) -
                          SourceMgr.getCharacterData(SortedBegin));
       DEBUG({
         StringRef OldText(SourceMgr.getCharacterData(Begin),
                           SourceMgr.getCharacterData(End) -
                               SourceMgr.getCharacterData(Begin));
         llvm::dbgs() << "Replacing '" << OldText << "' with '" << Text << "'\n";
       });
       auto Range = CharSourceRange::getCharRange(Begin, End);
       auto Err = Fixes->add(tooling::Replacement(SourceMgr, Range, Text));
       if (Err) {
         llvm::errs() << "Error while sorting using declarations: "
                      << llvm::toString(std::move(Err)) << "\n";
       }
     }
+  }
   UsingDeclarations->clear();
+  *BlockAffected = false;
 }
 
 } // namespace
 
 UsingDeclarationsSorter::UsingDeclarationsSorter(const Environment &Env,
                                                  const FormatStyle &Style)
     : TokenAnalyzer(Env, Style) {}
 
 tooling::Replacements UsingDeclarationsSorter::analyze(
     TokenAnnotator &Annotator, SmallVectorImpl<AnnotatedLine *> &AnnotatedLines,
     FormatTokenLexer &Tokens) {
   const SourceManager &SourceMgr = Env.getSourceManager();
   AffectedRangeMgr.computeAffectedLines(AnnotatedLines.begin(),
                                         AnnotatedLines.end());
   tooling::Replacements Fixes;
   SmallVector<UsingDeclaration, 4> UsingDeclarations;
+  bool BlockAffected = false;
   for (size_t I = 0, E = AnnotatedLines.size(); I != E; ++I) {
-    if (!AnnotatedLines[I]->Affected || AnnotatedLines[I]->InPPDirective ||
+    if (AnnotatedLines[I]->InPPDirective ||
         !AnnotatedLines[I]->startsWith(tok::kw_using) ||
         AnnotatedLines[I]->First->Finalized) {
-      endUsingDeclarationBlock(&UsingDeclarations, SourceMgr, &Fixes);
+      endUsingDeclarationBlock(&UsingDeclarations, &BlockAffected, SourceMgr,
+                               &Fixes);
       continue;
     }
     if (AnnotatedLines[I]->First->NewlinesBefore > 1)
-      endUsingDeclarationBlock(&UsingDeclarations, SourceMgr, &Fixes);
+      endUsingDeclarationBlock(&UsingDeclarations, &BlockAffected, SourceMgr,
+                               &Fixes);
     std::string Label = computeUsingDeclarationLabel(AnnotatedLines[I]->First);
     if (Label.empty()) {
-      endUsingDeclarationBlock(&UsingDeclarations, SourceMgr, &Fixes);
+      endUsingDeclarationBlock(&UsingDeclarations, &BlockAffected, SourceMgr,
+                               &Fixes);
       continue;
     }
     UsingDeclarations.push_back(UsingDeclaration(AnnotatedLines[I], Label));
+    BlockAffected = BlockAffected || AnnotatedLines[I]->Affected;
   }
-  endUsingDeclarationBlock(&UsingDeclarations, SourceMgr, &Fixes);
+  endUsingDeclarationBlock(&UsingDeclarations, &BlockAffected, SourceMgr,
+                           &Fixes);
   return Fixes;
 }
 
 } // namespace format
 } // namespace clang

unittests/Format/UsingDeclarationsSorterTest.cpp

EXPECT_EQ("// clang-format off\n"
                                   "using b;\n"
                                   "using a;\n"
                                   "// clang-format on\n"
                                   "using d;\n"
                                   "using c;"));
 }
 
 TEST_F(UsingDeclarationsSorterTest, SortsPartialRangeOfUsingDeclarations) {
-  EXPECT_EQ("using b;\n"
-            "using a;\n"
+  // Sorts the whole block of using declarations surrounding the range.
+  EXPECT_EQ("using a;\n"
+            "using b;\n"
             "using c;",
             sortUsingDeclarations("using b;\n"
                                   "using c;\n" // starts at offset 10
                                   "using a;",
                                   {tooling::Range(10, 15)}));
+  EXPECT_EQ("using a;\n"
+            "using b;\n"
+            "using c;\n"
+            "using A = b;",
+            sortUsingDeclarations("using b;\n"
+                                  "using c;\n" // starts at offset 10
+                                  "using a;\n"
+                                  "using A = b;",
+                                  {tooling::Range(10, 15)}));
+
+  EXPECT_EQ("using d;\n"
+            "using c;\n"
+            "\n"
+            "using a;\n"
+            "using b;\n"
+            "\n"
+            "using f;\n"
+            "using e;",
+            sortUsingDeclarations("using d;\n"
+                                  "using c;\n"
+                                  "\n"
+                                  "using b;\n" // starts at offset 19
+                                  "using a;\n"
+                                  "\n"
+                                  "using f;\n"
+                                  "using e;",
+                                  {tooling::Range(19, 1)}));
 }
 
 } // end namespace
 } // end namespace format
 } // end namespace clang