[tooling] Add support for fixits that indicate code will need reformatting

[njames93]

Summary

Implementing a FixItHint which indicates that code needs reformatting but not changing.

This is achieved by creating a FixItHint where the RemoveRange is equal to the InsertFromRange.
This means any dependent clients wont need updating as that is essentially a no-op. For clients that (are updated to) handle it, it signifies that code needs reformatting.
Currently this has been added to clang-tidy but for any refactoring tool that uses the Replacements its rather trivial to include support.
Generally this should be tacked onto other fix-its that maybe span multiple lines or insert/remove braces.

[jingham]

IIUC, the expression parser part of this change suppresses any Fixits that are clang-tidy type rewrites, is that right? If so that is indeed the correct behavior. But the fact that this change implements that behavior is entirely non-obvious at the call site. Could you either add a comment here explaining to point of the test or maybe wrap the test in a method on the FixItHint class so that it's self documenting?

[njames93]

In D91103#2384048, @jingham wrote:

IIUC, the expression parser part of this change suppresses any Fixits that are clang-tidy type rewrites, is that right? If so that is indeed the correct behavior. But the fact that this change implements that behavior is entirely non-obvious at the call site. Could you either add a comment here explaining to point of the test or maybe wrap the test in a method on the FixItHint class so that it's self documenting?

Hmm thats a good point, maybe a method inside FixItHint like,

bool isReformatRange() const {
  return InsertFromRange() == RemoveRange();
}

[njames93]

Add isReformatFixit method to FixItHint to better express intent.

[njames93]

An example of this in action is D91149

[njames93]

Teach clangd to ignore these fix-its.

[kadircet]

clang-tools-extra/clangd/Diagnostics.cpp

636

in theory yes, as we have access to source manager, we can fetch file contents and create formatted replacements (see cleanupAndFormat). but formatting those fixes can imply significant delays on clangd's diagnostic cycles (if there are many of those), that's the reason why we currently don't format fixits.

637

rather than doing an extra loop, can we just skip those in the for loop below ?

[njames93]

clang-tools-extra/clangd/Diagnostics.cpp

636

I mean if clangd is extended to support async code actions for diagnostics instead of just using the inline extension. Having said that, if that were the case, this would probably not be where that support is implemented.

637

We could, however that would result in messier code further down when trying to build a synthetic message.

[njames93]

Removed the first loop for clangd diagnostic, turns out it didnt make the following code that much messier.

[kadircet]

clang-tools-extra/clangd/Diagnostics.cpp

677

nit: you can check Edits.size() == 1 here, s/SingleFixIt/FixItForLastEdit and get rid of the optional.

[njames93]

Address nit by replacing optional.

clang-tools-extra/clang-tidy/ClangTidy.cpp

SmallVector<std::pair<SourceLocation, bool>, 4> FixLocations;
               }
             } else {
               CanBeApplied = true;
               ++AppliedFixes;
             }
             FixLoc = getLocation(FixAbsoluteFilePath, Repl.getOffset());
             FixLocations.push_back(std::make_pair(FixLoc, CanBeApplied));
           }
+          for (const auto &FR : FileAndReplacements.second.affected_ranges()) {
+            if (!FR.isValid())
+              continue;
+            SmallString<128> FixAbsoluteFilePath = FR.getFilePath();
+            Files.makeAbsolutePath(FixAbsoluteFilePath);
+            tooling::FileRange R(FixAbsoluteFilePath, FR.getRange());
+            Replacements &Replacements = FileReplacements[R.getFilePath()];
+            // This should never fail as there addReformatRange only returns an
+            // error if the files mismatch.
+            cantFail(Replacements.addReformatRange(R));
+          }
         }
       }
       reportFix(Diag, Error.Message.Fix);
     }
     for (auto Fix : FixLocations) {
       Diags.Report(Fix.first, Fix.second ? diag::note_fixit_applied
                                          : diag::note_fixit_failed);
     }

clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.cpp

void emitCodeContext(FullSourceLoc Loc, DiagnosticsEngine::Level Level,
 
     for (const auto &FixIt : Hints) {
       CharSourceRange Range = FixIt.RemoveRange;
       assert(Range.getBegin().isValid() && Range.getEnd().isValid() &&
              "Invalid range in the fix-it hint.");
       assert(Range.getBegin().isFileID() && Range.getEnd().isFileID() &&
              "Only file locations supported in fix-it hints.");
 
-      tooling::Replacement Replacement(Loc.getManager(), Range,
-                                       FixIt.CodeToInsert);
-      llvm::Error Err =
-          DiagWithFix->Fix[Replacement.getFilePath()].add(Replacement);
-      // FIXME: better error handling (at least, don't let other replacements be
-      // applied).
+      tooling::FileRange FRange(Loc.getManager(), Range);
+      tooling::Replacements &Replacements =
+          DiagWithFix->Fix[FRange.getFilePath()];
+
+      if (FixIt.isReformatFixit()) {
+        cantFail(Replacements.addReformatRange(FRange));
+      } else {
+        tooling::Replacement Replacement(FRange, FixIt.CodeToInsert);
+        llvm::Error Err = Replacements.add(Replacement);
+        // FIXME: better error handling (at least, don't let other replacements
+        // be applied).
         if (Err) {
           llvm::errs() << "Fix conflicts with existing fix! "
                        << llvm::toString(std::move(Err)) << "\n";
           assert(false && "Fix conflicts with existing fix!");
         }
       }
     }
+  }
 
   void emitIncludeLocation(FullSourceLoc Loc, PresumedLoc PLoc) override {}
 
   void emitImportLocation(FullSourceLoc Loc, PresumedLoc PLoc,
                           StringRef ModuleName) override {}
 
   void emitBuildingModuleLocation(FullSourceLoc Loc, PresumedLoc PLoc,
                                   StringRef ModuleName) override {}

clang-tools-extra/clangd/Diagnostics.cpp

void StoreDiags::HandleDiagnostic(DiagnosticsEngine::Level DiagLevel,
   auto AddFix = [&](bool SyntheticMessage) -> bool {
     assert(!Info.getFixItHints().empty() &&
            "diagnostic does not have attached fix-its");
     if (!InsideMainFile)
       return false;
 
     // Copy as we may modify the ranges.
     auto FixIts = Info.getFixItHints().vec();
+
+    // If this doesn't store a value, there are no code modifying fix-its
+    // attached to this diagnostic.
+    // If it stores a nullptr, there are multiple code modifiying fix-its,
+    // otherwise it stores a pointer to the only code modifying fix-it attacahed
+    // to this diagnostic.
+    Optional<const FixItHint *> SingleFixIt;
+
     llvm::SmallVector<TextEdit, 1> Edits;
     for (auto &FixIt : FixIts) {
+
+      // These don't modify code and aren't supported by clangd.
+      if (FixIt.isReformatFixit())
+        continue;
+
+      SingleFixIt = SingleFixIt ? nullptr : &FixIt;
       // Allow fixits within a single macro-arg expansion to be applied.
       // This can be incorrect if the argument is expanded multiple times in
       // different contexts. Hopefully this is rare!
       if (FixIt.RemoveRange.getBegin().isMacroID() &&
           FixIt.RemoveRange.getEnd().isMacroID() &&
           SM.getFileID(FixIt.RemoveRange.getBegin()) ==
               SM.getFileID(FixIt.RemoveRange.getEnd())) {
         FixIt.RemoveRange = CharSourceRange(
             {SM.getTopMacroCallerLoc(FixIt.RemoveRange.getBegin()),
              SM.getTopMacroCallerLoc(FixIt.RemoveRange.getEnd())},
             FixIt.RemoveRange.isTokenRange());
       }
       // Otherwise, follow clang's behavior: no fixits in macros.
       if (FixIt.RemoveRange.getBegin().isMacroID() ||
           FixIt.RemoveRange.getEnd().isMacroID())
         return false;
       if (!isInsideMainFile(FixIt.RemoveRange.getBegin(), SM))
         return false;
       Edits.push_back(toTextEdit(FixIt, SM, *LangOpts));
     }
 
+    // If this has no value, then no code modifying fix-its were attached
+    if (!SingleFixIt)
+      return false;
+
     llvm::SmallString<64> Message;
     // If requested and possible, create a message like "change 'foo' to 'bar'".
-    if (SyntheticMessage && FixIts.size() == 1) {
-      const auto &FixIt = FixIts.front();
+    if (SyntheticMessage && *SingleFixIt) {
+      const auto &FixIt = **SingleFixIt;
       bool Invalid = false;
       llvm::StringRef Remove =
           Lexer::getSourceText(FixIt.RemoveRange, SM, *LangOpts, &Invalid);
       llvm::StringRef Insert = FixIt.CodeToInsert;
       if (!Invalid) {
         llvm::raw_svector_ostream M(Message);
         if (!Remove.empty() && !Insert.empty()) {
           M << "change '";

clang/include/clang/Basic/Diagnostic.h

public:
   /// Empty code modification hint, indicating that no code
   /// modification is known.
   FixItHint() = default;
 
   bool isNull() const {
     return !RemoveRange.isValid();
   }
 
+  /// Signifies that the code modification doesn't modify code, just a hint that
+  /// it should be reformatted.
+  bool isReformatFixit() const { return RemoveRange == InsertFromRange; }
+
   /// Create a code modification hint that inserts the given
   /// code string at a specific location.
   static FixItHint CreateInsertion(SourceLocation InsertionLoc,
                                    StringRef Code,
                                    bool BeforePreviousInsertions = false) {
     FixItHint Hint;
     Hint.RemoveRange =
       CharSourceRange::getCharRange(InsertionLoc, InsertionLoc);
static FixItHint CreateReplacement(CharSourceRange RemoveRange,
     Hint.CodeToInsert = std::string(Code);
     return Hint;
   }
 
   static FixItHint CreateReplacement(SourceRange RemoveRange,
                                      StringRef Code) {
     return CreateReplacement(CharSourceRange::getTokenRange(RemoveRange), Code);
   }
+
+  /// Creates a Fix that doesn't modify code, just hints that \p Range needs
+  /// reformatting.
+  static FixItHint CreateReformatFixIt(SourceRange Range) {
+    FixItHint Hint;
+    Hint.RemoveRange = Hint.InsertFromRange =
+        CharSourceRange::getCharRange(Range);
+    return Hint;
+  }
 };
 
 struct DiagnosticStorage {
   enum {
     /// The maximum number of arguments we can hold. We
     /// currently only support up to 10 arguments (%0-%9).
     ///
     /// A single diagnostic with more than that almost certainly has to

clang/include/clang/Basic/SourceLocation.h

public:
   void setBegin(SourceLocation b) { Range.setBegin(b); }
   void setEnd(SourceLocation e) { Range.setEnd(e); }
   void setTokenRange(bool TR) { IsTokenRange = TR; }
 
   bool isValid() const { return Range.isValid(); }
   bool isInvalid() const { return !isValid(); }
 };
 
+inline bool operator==(const CharSourceRange &LHS, const CharSourceRange &RHS) {
+  return LHS.isCharRange() == RHS.isCharRange() &&
+         LHS.getAsRange() == RHS.getAsRange();
+}
+
+inline bool operator!=(const CharSourceRange &LHS, const CharSourceRange &RHS) {
+  return !(LHS == RHS);
+}
+
 /// Represents an unpacked "presumed" location which can be presented
 /// to the user.
 ///
 /// A 'presumed' location can be modified by \#line and GNU line marker
 /// directives and is always the expansion point of a normal location.
 ///
 /// You can get a PresumedLoc from a SourceLocation with SourceManager.
 class PresumedLoc {

clang/include/clang/Tooling/Core/Replacement.h

public:
   }
   /// @}
 
 private:
   unsigned Offset = 0;
   unsigned Length = 0;
 };
 
+class FileRange {
+
+public:
+  FileRange();
+  FileRange(StringRef FilePath, Range RangeInFile);
+  FileRange(StringRef FilePath, unsigned Offset, unsigned Length);
+
+  FileRange(const SourceManager &Sources, SourceLocation Start,
+            unsigned Length);
+
+  FileRange(const SourceManager &Sources, const CharSourceRange &Range,
+            const LangOptions &LangOpts = LangOptions());
+
+  template <typename Node>
+  FileRange(const SourceManager &Sources, const Node &ASTNode,
+            const LangOptions &LangOpts = LangOptions());
+
+  StringRef getFilePath() const { return FilePath; }
+  const Range &getRange() const { return RangeInFile; }
+  unsigned getOffset() const { return RangeInFile.getOffset(); }
+  unsigned getLength() const { return RangeInFile.getLength(); }
+
+  bool isValid() const;
+
+private:
+  void setFromSourceLocation(const SourceManager &Sources, SourceLocation Start,
+                             unsigned Length);
+  void setFromSourceRange(const SourceManager &Sources,
+                          const CharSourceRange &Range,
+                          const LangOptions &LangOpts);
+
+  std::string FilePath;
+  Range RangeInFile;
+};
+
 /// A text replacement.
 ///
 /// Represents a SourceManager independent replacement of a range of text in a
 /// specific file.
 class Replacement {
 public:
   /// Creates an invalid (not applicable) replacement.
   Replacement();
 
+  Replacement(FileRange FileRange, StringRef ReplacementText);
+
   /// Creates a replacement of the range [Offset, Offset+Length) in
   /// FilePath with ReplacementText.
   ///
   /// \param FilePath A source file accessible via a SourceManager.
   /// \param Offset The byte offset of the start of the range in the file.
   /// \param Length The length of the range in bytes.
   Replacement(StringRef FilePath, unsigned Offset, unsigned Length,
               StringRef ReplacementText);
 
   /// Creates a Replacement of the range [Start, Start+Length) with
   /// ReplacementText.
   Replacement(const SourceManager &Sources, SourceLocation Start,
               unsigned Length, StringRef ReplacementText);
 
   /// Creates a Replacement of the given range with ReplacementText.
   Replacement(const SourceManager &Sources, const CharSourceRange &Range,
               StringRef ReplacementText,
               const LangOptions &LangOpts = LangOptions());
 
   /// Creates a Replacement of the node with ReplacementText.
   template <typename Node>
   Replacement(const SourceManager &Sources, const Node &NodeToReplace,
               StringRef ReplacementText,
-              const LangOptions &LangOpts = LangOptions());
+              const LangOptions &LangOpts = LangOptions())
+      : ReplacementRange(Sources, NodeToReplace, LangOpts),
+        ReplacementText(ReplacementText) {}
 
   /// Returns whether this replacement can be applied to a file.
   ///
   /// Only replacements that are in a valid file can be applied.
   bool isApplicable() const;
 
   /// Accessors.
   /// @{
-  StringRef getFilePath() const { return FilePath; }
+  StringRef getFilePath() const { return ReplacementRange.getFilePath(); }
   unsigned getOffset() const { return ReplacementRange.getOffset(); }
   unsigned getLength() const { return ReplacementRange.getLength(); }
   StringRef getReplacementText() const { return ReplacementText; }
+  const FileRange &getFileRange() const { return ReplacementRange; }
   /// @}
 
   /// Applies the replacement on the Rewriter.
   bool apply(Rewriter &Rewrite) const;
 
   /// Returns a human readable string representation.
   std::string toString() const;
 
 private:
-  void setFromSourceLocation(const SourceManager &Sources, SourceLocation Start,
-                             unsigned Length, StringRef ReplacementText);
-  void setFromSourceRange(const SourceManager &Sources,
-                          const CharSourceRange &Range,
-                          StringRef ReplacementText,
-                          const LangOptions &LangOpts);
-
-  std::string FilePath;
-  Range ReplacementRange;
+  FileRange ReplacementRange;
   std::string ReplacementText;
 };
 
 enum class replacement_error {
   fail_to_apply = 0,
   wrong_file_path,
   overlap_conflict,
   insert_conflict,
private:
   // A new replacement, which is to expected be added into a set of
   // replacements, that is causing problem.
   llvm::Optional<Replacement> NewReplacement;
 
   // An existing replacement in a replacements set that is causing problem.
   llvm::Optional<Replacement> ExistingReplacement;
 };
 
+/// Less-than operator between two Ranges.
+bool operator<(const Range &LHS, const Range &RHS);
+
+/// Less-than operator between two FileRanges.
+bool operator<(const FileRange &LHS, const FileRange &RHS);
+
+/// Equal-to operator between two FileRanges.
+bool operator==(const FileRange &LHS, const FileRange &RHS);
+
 /// Less-than operator between two Replacements.
 bool operator<(const Replacement &LHS, const Replacement &RHS);
 
 /// Equal-to operator between two Replacements.
 bool operator==(const Replacement &LHS, const Replacement &RHS);
 
 /// Maintains a set of replacements that are conflict-free.
 /// Two replacements are considered conflicts if they overlap or have the same
 /// offset (i.e. order-dependent).
 class Replacements {
 private:
   using ReplacementsImpl = std::set<Replacement>;
+  using AffectedImpl = llvm::SmallVector<FileRange, 0>;
 
 public:
   using const_iterator = ReplacementsImpl::const_iterator;
   using const_reverse_iterator = ReplacementsImpl::const_reverse_iterator;
 
   Replacements() = default;
 
   explicit Replacements(const Replacement &R) { Replaces.insert(R); }
public:
   ///    since either order gives (0, 5, "12321").
   /// 4. Replacement A(0, 3, "ab") and B(0, 3, "ab") are order-independent since
   ///    applying the same replacement twice is equivalent to applying it once.
   /// Replacements with offset UINT_MAX are special - we do not detect conflicts
   /// for such replacements since users may add them intentionally as a special
   /// category of replacements.
   llvm::Error add(const Replacement &R);
 
+  llvm::Error addReformatRange(const FileRange &FormatRange);
+
   /// Merges \p Replaces into the current replacements. \p Replaces
   /// refers to code after applying the current replacements.
   LLVM_NODISCARD Replacements merge(const Replacements &Replaces) const;
 
   // Returns the affected ranges in the changed code.
   std::vector<Range> getAffectedRanges() const;
 
   // Returns the new offset in the code after replacements being applied.
   // Note that if there is an insertion at Offset in the current replacements,
   // \p Offset will be shifted to Offset + Length in inserted text.
   unsigned getShiftedCodePosition(unsigned Position) const;
 
   unsigned size() const { return Replaces.size(); }
 
+  llvm::Optional<StringRef> getFilePath() const;
+
   void clear() { Replaces.clear(); }
 
   bool empty() const { return Replaces.empty(); }
 
   const_iterator begin() const { return Replaces.begin(); }
 
   const_iterator end() const { return Replaces.end(); }
 
   const_reverse_iterator rbegin() const  { return Replaces.rbegin(); }
 
   const_reverse_iterator rend() const { return Replaces.rend(); }
 
+  AffectedImpl::const_iterator affected_ranges_begin() const {
+    return Affected.begin();
+  }
+
+  AffectedImpl::const_iterator affected_ranges_end() const {
+    return Affected.end();
+  }
+
+  llvm::iterator_range<AffectedImpl::const_iterator> affected_ranges() const {
+    return {affected_ranges_begin(), affected_ranges_end()};
+  }
+
   bool operator==(const Replacements &RHS) const {
-    return Replaces == RHS.Replaces;
+    return Replaces == RHS.Replaces && Affected == RHS.Affected;
   }
 
 private:
   Replacements(const_iterator Begin, const_iterator End)
       : Replaces(Begin, End) {}
 
   // Returns `R` with new range that refers to code after `Replaces` being
   // applied.
   Replacement getReplacementInChangedCode(const Replacement &R) const;
 
   // Returns a set of replacements that is equivalent to the current
   // replacements by merging all adjacent replacements. Two sets of replacements
   // are considered equivalent if they have the same effect when they are
   // applied.
   Replacements getCanonicalReplacements() const;
 
   // If `R` and all existing replacements are order-indepedent, then merge it
   // with `Replaces` and returns the merged replacements; otherwise, returns an
   // error.
   llvm::Expected<Replacements>
   mergeIfOrderIndependent(const Replacement &R) const;
 
   ReplacementsImpl Replaces;
+  AffectedImpl Affected;
 };
 
 /// Apply all replacements in \p Replaces to the Rewriter \p Rewrite.
 ///
 /// Replacement applications happen independently of the success of
 /// other applications.
 ///
 /// \returns true if all replacements apply. false otherwise.

 /// If there are multiple <File, Replacements> pairs with the same file
 /// entry, we only keep one pair and discard the rest.
 /// If a file does not exist, its corresponding replacements will be ignored.
 std::map<std::string, Replacements> groupReplacementsByFile(
     FileManager &FileMgr,
     const std::map<std::string, Replacements> &FileToReplaces);
 
 template <typename Node>
-Replacement::Replacement(const SourceManager &Sources,
-                         const Node &NodeToReplace, StringRef ReplacementText,
-                         const LangOptions &LangOpts) {
+FileRange::FileRange(const SourceManager &Sources, const Node &ASTNode,
   const CharSourceRange Range =
-      CharSourceRange::getTokenRange(NodeToReplace->getSourceRange());
-  setFromSourceRange(Sources, Range, ReplacementText, LangOpts);
+      CharSourceRange::getTokenRange(ASTNode->getSourceRange());
+  setFromSourceRange(Sources, Range, LangOpts);
 }
 
 } // namespace tooling
 
 } // namespace clang
 
 #endif // LLVM_CLANG_TOOLING_CORE_REPLACEMENT_H

clang/lib/Frontend/DiagnosticRenderer.cpp

public:
   }
 };
 
 } // namespace
 
 static void mergeFixits(ArrayRef<FixItHint> FixItHints,
                         const SourceManager &SM, const LangOptions &LangOpts,
                         SmallVectorImpl<FixItHint> &MergedFixits) {
+  // Never expext more than one range per diag.
+  SmallVector<const FixItHint *, 1> Reformats;
   edit::Commit commit(SM, LangOpts);
   for (const auto &Hint : FixItHints)
     if (Hint.CodeToInsert.empty()) {
-      if (Hint.InsertFromRange.isValid())
+      if (Hint.InsertFromRange.isValid()) {
+        if (Hint.isReformatFixit())
+          Reformats.push_back(&Hint);
+        else
           commit.insertFromRange(Hint.RemoveRange.getBegin(),
                                  Hint.InsertFromRange, /*afterToken=*/false,
                                  Hint.BeforePreviousInsertions);
-      else
+      } else
         commit.remove(Hint.RemoveRange);
     } else {
       if (Hint.RemoveRange.isTokenRange() ||
           Hint.RemoveRange.getBegin() != Hint.RemoveRange.getEnd())
         commit.replace(Hint.RemoveRange, Hint.CodeToInsert);
       else
         commit.insert(Hint.RemoveRange.getBegin(), Hint.CodeToInsert,
                     /*afterToken=*/false, Hint.BeforePreviousInsertions);
     }
 
   edit::EditedSource Editor(SM, LangOpts);
   if (Editor.commit(commit)) {
     FixitReceiver Rec(MergedFixits);
     Editor.applyRewrites(Rec);
   }
+  for (const auto *Hint : Reformats) {
+    MergedFixits.push_back(*Hint);
+  }
 }
 
 void DiagnosticRenderer::emitDiagnostic(FullSourceLoc Loc,
                                         DiagnosticsEngine::Level Level,
                                         StringRef Message,
                                         ArrayRef<CharSourceRange> Ranges,
                                         ArrayRef<FixItHint> FixItHints,
                                         DiagOrStoredDiag D) {

clang/lib/Frontend/Rewrite/FixItRewriter.cpp

void FixItRewriter::HandleDiagnostic(DiagnosticsEngine::Level DiagLevel,
   if (DiagLevel >= DiagnosticsEngine::Error && FixItOpts->FixOnlyWarnings) {
     ++NumFailures;
     return;
   }
 
   // Make sure that we can perform all of the modifications we
   // in this diagnostic.
   edit::Commit commit(Editor);
-  for (unsigned Idx = 0, Last = Info.getNumFixItHints();
-       Idx < Last; ++Idx) {
-    const FixItHint &Hint = Info.getFixItHint(Idx);
-
+  unsigned NonApplicableFixits = 0;
+  for (const FixItHint &Hint : Info.getFixItHints()) {
     if (Hint.CodeToInsert.empty()) {
-      if (Hint.InsertFromRange.isValid())
+      if (Hint.InsertFromRange.isValid()) {
+        if (Hint.isReformatFixit())
+          ++NonApplicableFixits;
+        else
           commit.insertFromRange(Hint.RemoveRange.getBegin(),
                                  Hint.InsertFromRange, /*afterToken=*/false,
                                  Hint.BeforePreviousInsertions);
-      else
+      } else
         commit.remove(Hint.RemoveRange);
     } else {
       if (Hint.RemoveRange.isTokenRange() ||
           Hint.RemoveRange.getBegin() != Hint.RemoveRange.getEnd())
         commit.replace(Hint.RemoveRange, Hint.CodeToInsert);
       else
         commit.insert(Hint.RemoveRange.getBegin(), Hint.CodeToInsert,
                     /*afterToken=*/false, Hint.BeforePreviousInsertions);
     }
   }
-  bool CanRewrite = Info.getNumFixItHints() > 0 && commit.isCommitable();
+  bool CanRewrite =
+      Info.getNumFixItHints() > NonApplicableFixits && commit.isCommitable();
 
   if (!CanRewrite) {
     if (Info.getNumFixItHints() > 0)
       Diag(Info.getLocation(), diag::note_fixit_in_macro);
 
     // If this was an error, refuse to perform any rewriting.
     if (DiagLevel >= DiagnosticsEngine::Error) {
       if (++NumFailures == 1)

clang/lib/Tooling/Core/Replacement.cpp

 #include "clang/Basic/FileManager.h"
 #include "clang/Basic/FileSystemOptions.h"
 #include "clang/Basic/SourceLocation.h"
 #include "clang/Basic/SourceManager.h"
 #include "clang/Lex/Lexer.h"
 #include "clang/Rewrite/Core/RewriteBuffer.h"
 #include "clang/Rewrite/Core/Rewriter.h"
 #include "llvm/ADT/IntrusiveRefCntPtr.h"
+#include "llvm/ADT/Optional.h"
 #include "llvm/ADT/SmallPtrSet.h"
 #include "llvm/ADT/StringRef.h"
 #include "llvm/Support/Error.h"
 #include "llvm/Support/ErrorHandling.h"
 #include "llvm/Support/MemoryBuffer.h"
 #include "llvm/Support/VirtualFileSystem.h"
 #include "llvm/Support/raw_ostream.h"
 #include <algorithm>
 #include <cassert>
 #include <limits>
 #include <map>
 #include <string>
 #include <utility>
 #include <vector>
 
 using namespace clang;
 using namespace tooling;
 
 static const char * const InvalidLocation = "";
 
-Replacement::Replacement() : FilePath(InvalidLocation) {}
+FileRange::FileRange() : FilePath(InvalidLocation), RangeInFile(0, 0) {}
+
+FileRange::FileRange(StringRef FilePath, Range RangeInFile)
+    : FilePath(FilePath), RangeInFile(RangeInFile) {}
+FileRange::FileRange(StringRef FilePath, unsigned Offset, unsigned Length)
+    : FileRange(FilePath, Range(Offset, Length)) {}
+
+FileRange::FileRange(const SourceManager &Sources, SourceLocation Start,
+                     unsigned Length) {
+  setFromSourceLocation(Sources, Start, Length);
+}
+
+FileRange::FileRange(const SourceManager &Sources, const CharSourceRange &Range,
+                     const LangOptions &LangOpts) {
+  setFromSourceRange(Sources, Range, LangOpts);
+}
+
+bool FileRange::isValid() const { return FilePath != InvalidLocation; }
+
+void FileRange::setFromSourceLocation(const SourceManager &Sources,
+                                      SourceLocation Start, unsigned Length) {
+  const std::pair<FileID, unsigned> DecomposedLocation =
+      Sources.getDecomposedLoc(Start);
+  const FileEntry *Entry = Sources.getFileEntryForID(DecomposedLocation.first);
+  this->FilePath = std::string(Entry ? Entry->getName() : InvalidLocation);
+  this->RangeInFile = {DecomposedLocation.second, Length};
+}
+
+Replacement::Replacement() : ReplacementRange() {}
+
+Replacement::Replacement(FileRange FileRange, StringRef ReplacementText)
+    : ReplacementRange(FileRange), ReplacementText(ReplacementText) {}
 
 Replacement::Replacement(StringRef FilePath, unsigned Offset, unsigned Length,
                          StringRef ReplacementText)
-    : FilePath(std::string(FilePath)), ReplacementRange(Offset, Length),
-      ReplacementText(std::string(ReplacementText)) {}
+    : Replacement(FileRange(FilePath, Offset, Length), ReplacementText) {}
 
 Replacement::Replacement(const SourceManager &Sources, SourceLocation Start,
-                         unsigned Length, StringRef ReplacementText) {
-  setFromSourceLocation(Sources, Start, Length, ReplacementText);
-}
+                         unsigned Length, StringRef ReplacementText)
+    : Replacement(FileRange(Sources, Start, Length), ReplacementText) {}
 
 Replacement::Replacement(const SourceManager &Sources,
                          const CharSourceRange &Range,
-                         StringRef ReplacementText,
-                         const LangOptions &LangOpts) {
-  setFromSourceRange(Sources, Range, ReplacementText, LangOpts);
-}
+                         StringRef ReplacementText, const LangOptions &LangOpts)
+    : Replacement(FileRange(Sources, Range, LangOpts), ReplacementText) {}
 
-bool Replacement::isApplicable() const {
-  return FilePath != InvalidLocation;
-}
+bool Replacement::isApplicable() const { return ReplacementRange.isValid(); }
 
 bool Replacement::apply(Rewriter &Rewrite) const {
   SourceManager &SM = Rewrite.getSourceMgr();
-  auto Entry = SM.getFileManager().getFile(FilePath);
+  auto Entry = SM.getFileManager().getFile(getFilePath());
   if (!Entry)
     return false;
 
   FileID ID = SM.getOrCreateFileID(*Entry, SrcMgr::C_User);
   const SourceLocation Start =
     SM.getLocForStartOfFile(ID).
     getLocWithOffset(ReplacementRange.getOffset());
   // ReplaceText returns false on success.
   // ReplaceText only fails if the source location is not a file location, in
   // which case we already returned false earlier.
   bool RewriteSucceeded = !Rewrite.ReplaceText(
       Start, ReplacementRange.getLength(), ReplacementText);
   assert(RewriteSucceeded);
   return RewriteSucceeded;
 }
 
 std::string Replacement::toString() const {
   std::string Result;
   llvm::raw_string_ostream Stream(Result);
-  Stream << FilePath << ": " << ReplacementRange.getOffset() << ":+"
-         << ReplacementRange.getLength() << ":\"" << ReplacementText << "\"";
+  Stream << getFilePath() << ": " << getOffset() << ":+" << getLength() << ":\""
+         << ReplacementText << "\"";
   return Stream.str();
 }
 
 namespace clang {
 namespace tooling {
 
-bool operator<(const Replacement &LHS, const Replacement &RHS) {
+bool operator<(const Range &LHS, const Range &RHS) {
   if (LHS.getOffset() != RHS.getOffset())
     return LHS.getOffset() < RHS.getOffset();
-
-  if (LHS.getLength() != RHS.getLength())
-    return LHS.getLength() < RHS.getLength();
+}
 
-  if (LHS.getFilePath() != RHS.getFilePath())
-    return LHS.getFilePath() < RHS.getFilePath();
+bool operator<(const FileRange &LHS, const FileRange &RHS) {
+  if (!(LHS.getRange() == RHS.getRange()))
+    return LHS.getRange() < RHS.getRange();
+}
+
+bool operator==(const FileRange &LHS, const FileRange &RHS) {
+  return LHS.getRange() == RHS.getRange() &&
+         LHS.getFilePath() == RHS.getFilePath();
+}
+
+bool operator<(const Replacement &LHS, const Replacement &RHS) {
+  if (!(LHS.getFileRange() == RHS.getFileRange()))
+    return LHS.getFileRange() < RHS.getFileRange();
   return LHS.getReplacementText() < RHS.getReplacementText();
 }
 
 bool operator==(const Replacement &LHS, const Replacement &RHS) {
-  return LHS.getOffset() == RHS.getOffset() &&
-         LHS.getLength() == RHS.getLength() &&
-         LHS.getFilePath() == RHS.getFilePath() &&
+  return LHS.getFileRange() == RHS.getFileRange() &&
          LHS.getReplacementText() == RHS.getReplacementText();
 }
 
 } // namespace tooling
 } // namespace clang
 
-void Replacement::setFromSourceLocation(const SourceManager &Sources,
-                                        SourceLocation Start, unsigned Length,
-                                        StringRef ReplacementText) {
-  const std::pair<FileID, unsigned> DecomposedLocation =
-      Sources.getDecomposedLoc(Start);
-  const FileEntry *Entry = Sources.getFileEntryForID(DecomposedLocation.first);
-  this->FilePath = std::string(Entry ? Entry->getName() : InvalidLocation);
-  this->ReplacementRange = Range(DecomposedLocation.second, Length);
-  this->ReplacementText = std::string(ReplacementText);
-}
-
 // FIXME: This should go into the Lexer, but we need to figure out how
 // to handle ranges for refactoring in general first - there is no obvious
 // good way how to integrate this into the Lexer yet.
 static int getRangeSize(const SourceManager &Sources,
                         const CharSourceRange &Range,
                         const LangOptions &LangOpts) {
   SourceLocation SpellingBegin = Sources.getSpellingLoc(Range.getBegin());
   SourceLocation SpellingEnd = Sources.getSpellingLoc(Range.getEnd());
   std::pair<FileID, unsigned> Start = Sources.getDecomposedLoc(SpellingBegin);
   std::pair<FileID, unsigned> End = Sources.getDecomposedLoc(SpellingEnd);
   if (Start.first != End.first) return -1;
   if (Range.isTokenRange())
     End.second += Lexer::MeasureTokenLength(SpellingEnd, Sources, LangOpts);
   return End.second - Start.second;
 }
 
-void Replacement::setFromSourceRange(const SourceManager &Sources,
-                                     const CharSourceRange &Range,
-                                     StringRef ReplacementText,
-                                     const LangOptions &LangOpts) {
+void FileRange::setFromSourceRange(const SourceManager &Sources,
   setFromSourceLocation(Sources, Sources.getSpellingLoc(Range.getBegin()),
-                        getRangeSize(Sources, Range, LangOpts),
-                        ReplacementText);
+                        getRangeSize(Sources, Range, LangOpts));
 }
 
 Replacement
 Replacements::getReplacementInChangedCode(const Replacement &R) const {
   unsigned NewStart = getShiftedCodePosition(R.getOffset());
   unsigned NewEnd = getShiftedCodePosition(R.getOffset() + R.getLength());
   return Replacement(R.getFilePath(), NewStart, NewEnd - NewStart,
                      R.getReplacementText());
Replacements::mergeIfOrderIndependent(const Replacement &R) const {
   // produced above, we need to compare replacements in canonical forms.
   if (MergeShiftedRs.getCanonicalReplacements() ==
       MergeShiftedReplaces.getCanonicalReplacements())
     return MergeShiftedRs;
   return llvm::make_error<ReplacementError>(replacement_error::overlap_conflict,
                                             R, *Replaces.begin());
 }
 
+llvm::Optional<StringRef> Replacements::getFilePath() const {
+  if (!Replaces.empty())
+    return Replaces.begin()->getFilePath();
+  if (!Affected.empty())
+    return Affected.front().getFilePath();
+  return None;
+}
+
+llvm::Error Replacements::addReformatRange(const FileRange &FormatRange) {
+  if (llvm::Optional<StringRef> Filepath = getFilePath()) {
+    if (FormatRange.getFilePath() != Filepath)
+      return llvm::make_error<ReplacementError>(
+          replacement_error::wrong_file_path);
+  }
+  Affected.push_back(FormatRange);
+  return llvm::Error::success();
+}
+
 llvm::Error Replacements::add(const Replacement &R) {
   // Check the file path.
-  if (!Replaces.empty() && R.getFilePath() != Replaces.begin()->getFilePath())
+  if (llvm::Optional<StringRef> Filepath = getFilePath()) {
+    if (R.getFilePath() != Filepath)
       return llvm::make_error<ReplacementError>(
           replacement_error::wrong_file_path, R, *Replaces.begin());
+  }
 
   // Special-case header insertions.
   if (R.getOffset() == std::numeric_limits<unsigned>::max()) {
     Replaces.insert(R);
     return llvm::Error::success();
   }
 
   // This replacement cannot conflict with replacements that end before
std::vector<Range> Replacements::getAffectedRanges() const {
   std::vector<Range> ChangedRanges;
   int Shift = 0;
   for (const auto &R : Replaces) {
     unsigned Offset = R.getOffset() + Shift;
     unsigned Length = R.getReplacementText().size();
     Shift += Length - R.getLength();
     ChangedRanges.push_back(Range(Offset, Length));
   }
+  for (const auto &A : Affected) {
+    unsigned Start = getShiftedCodePosition(A.getOffset());
+    unsigned End = getShiftedCodePosition(A.getOffset() + A.getLength());
+    ChangedRanges.push_back(Range(Start, End - Start));
+  }
   return combineAndSortRanges(ChangedRanges);
 }
 
 unsigned Replacements::getShiftedCodePosition(unsigned Position) const {
   unsigned Offset = 0;
   for (const auto &R : Replaces) {
     if (R.getOffset() + R.getLength() <= Position) {
       Offset += R.getReplacementText().size() - R.getLength();

lldb/source/Plugins/ExpressionParser/Clang/ClangExpressionParser.cpp

ClangExpressionParser::GetClangTargetABI(const ArchSpec &target_arch) {
   return abi;
 }
 
 /// Applies the given Fix-It hint to the given commit.
 static void ApplyFixIt(const FixItHint &fixit, clang::edit::Commit &commit) {
   // This is cobbed from clang::Rewrite::FixItRewriter.
   if (fixit.CodeToInsert.empty()) {
     if (fixit.InsertFromRange.isValid()) {
+      if (!fixit.isReformatFixit())
         commit.insertFromRange(fixit.RemoveRange.getBegin(),
                                fixit.InsertFromRange, /*afterToken=*/false,
                                fixit.BeforePreviousInsertions);
       return;
     }
     commit.remove(fixit.RemoveRange);
     return;
   }
   if (fixit.RemoveRange.isTokenRange() ||
       fixit.RemoveRange.getBegin() != fixit.RemoveRange.getEnd()) {
     commit.replace(fixit.RemoveRange, fixit.CodeToInsert);