Introducing clang::tooling::AtomicChange for refactoring tools.

[ioeric]

Summary

An AtomicChange is used to create and group a set of source edits, e.g.
replacements or header insertions. Edits in an AtomicChange should be related,
e.g. replacements for the same type reference and the corresponding header
insertion/deletion.

An AtomicChange is uniquely identified by a key position and will either be
fully applied or not applied at all. The key position should be the location
of the key syntactical element that is being changed, e.g. the call to a
refactored method.

Next step: add a tool that applies AtomicChange.

[ioeric]

Ping.

Daniel, could you take a look? Manuel and I would like your opinion on this.

[djasper]

include/clang/Tooling/Refactoring/EditList.h

41 ↗

(On Diff #79099)

I wonder whether we should always use a SourceLocation as key or whether we want to leave this up to the users. E.g. we could make this take a string parameter and provide a

string getKeyForLocation(const SourceManager &SM, SourceLocation KeyPosition);

43 ↗

(On Diff #79099)

Does this need to be public? Specifically, couldn't the YamlTraits create a NormalizedEditList and then convertFromYAML can create the EditList object, which should have access to a private constructor?

82–94 ↗

(On Diff #79099)

Do we currently actually need these functions? They seem a bit dangerous.

117 ↗

(On Diff #79099)

Maybe add a short description of what "Key" contains. The others are pretty intuitive.

lib/Tooling/Refactoring/EditList.cpp

25 ↗

(On Diff #79099)

I don't understand (without more comments) why this is normalized/denormalized.

unittests/Tooling/RefactoringTest.cpp

1113 ↗

(On Diff #79099)

I think these need to be ASSERT_TRUE.

[ioeric]

• Addressed review comments.

include/clang/Tooling/Refactoring/EditList.h

41 ↗

(On Diff #79099)

I think the key idea of EditList is that it groups a set of edits around a key position. For refactoring, using position as key makes sense to me (since all edits will be around some position), and I couldn't think of other things that are good to be a key here.

43 ↗

(On Diff #79099)

You are right! Make this private now.

82–94 ↗

(On Diff #79099)

I think these functions are helpful. Insertion conflict is by far the most common conflict I've seen, and several users have asked for ways to resolve this. And generally, resolving such conflict is not straight-forward. Also notice that these interfaces only resolve insertion conflict, other conflicts (e.g. overlaps) will still raise errors.

Although inserting all texts at one location as a single replacement is preferred, I've seen several tools that use these in some way, i.e. generating all insertions at the same location is hard or impossible.

[klimek]

include/clang/Tooling/Refactoring/EditList.h

41 ↗

(On Diff #79099)

An idea would be an edit that just inserts multiple headers, but does not directly touch a source location (thus, we might want to key things off of files?).

[ioeric]

include/clang/Tooling/Refactoring/EditList.h

41 ↗

(On Diff #79099)

We could make the key position the start of the file in this case. But looks like a customize-able key could potentially enable more opportunities, I'll go with Daniel's suggestion. Thanks!

[ioeric]

• Make key customize-able.

include/clang/Tooling/Refactoring/EditList.h

41 ↗

(On Diff #79099)

So I tried the getKeyForLocation way, but we still need FilePath in the constructor to fully initialize an EditList.

I think it might be better if we keep the current constructor (since this will be the most-commonly used one IMO) and adds another constructor that takes FilePath and Key.

[djasper]

include/clang/Tooling/Refactoring/EditList.h

41 ↗

(On Diff #79099)

Ah, but you might want to include more details in the Key, e.g. the branch or source revision. My point is that the user might be better equipped to know what a valid key might be. But I am happy to keep it as is and add another constructor where we pass in a key when needed.

82–94 ↗

(On Diff #79099)

I still have several concerns:

1. This interface actually exposes two different layers: It has special function to insert text and headers and also the functions that add or merge replacements. IMO, that's not ideal.
2. There is now no function insert() if users actually want to get the error for a conflict. Yes, they can then instead use addReplacement, but again, that's a completely separate layer of the interface.
3. The logic of before after AND that this only applies to two conflicting insertions is a bit hard to explain.

Not sure what the right trade-off for all of these is. Maybe we should just remove addReplacement and mergeReplacements and instead add a replace() function? Maybe we should just have a single insert() function with an optional fourth parameter that can be used to control conflict resolution.

lib/Tooling/Refactoring/EditList.cpp

164 ↗

(On Diff #79728)

Don't duplicate this code. Add an internal function that takes an extra parameter that controls conflict resolution.

[ioeric]

include/clang/Tooling/Refactoring/EditList.h

82–94 ↗

(On Diff #79099)

replace + insert sounds like a good way to go. Do we want to support resolving general conflicts besides insertions though?

[djasper]

include/clang/Tooling/Refactoring/EditList.h

82–94 ↗

(On Diff #79099)

I think that might not be necessary. If so, we might need to see a few use cases to be sure to get it right.

[ioeric]

• Added replace() and insert() to replace current replacement interfaces.

[ioeric]

• s/EditList/AtomicChange/

[alexander-shaposhnikov]

include/clang/Tooling/Refactoring/AtomicChange.h

59 ↗

(On Diff #90011)

i assume i might be missing smth - why here and above (in getKey, getFilePath, getError) std::string is returned by value ?

[alexander-shaposhnikov]

lib/Tooling/Refactoring/AtomicChange.cpp

35 ↗

(On Diff #90011)

if i am not mistaken this can be done in the intialization list:

Replaces(E.getReplacements().begin(), E.getReplacements().end())

[ioeric]

include/clang/Tooling/Refactoring/AtomicChange.h

59 ↗

(On Diff #90011)

Otherwise, users would need to worry about the lifetimes of the object. And these methods are not expected to be called intensively, so performance is not an issue.

lib/Tooling/Refactoring/AtomicChange.cpp

35 ↗

(On Diff #90011)

You are right.

[ioeric]

• Addressed comments.

[alexander-shaposhnikov]

include/clang/Tooling/Refactoring/AtomicChange.h

59 ↗

(On Diff #90011)

i see, thanks for the explanation, probably not particularly important
i thought that if a caller wanted to get a copy / take ownership
it would just accept it by value, i.e.

auto path = change.getFilePath() /* assuming getFilePath returns const ref */

At the same time if for example we need to check the filepath (or just print it),
not copies would be required:

llvm::errs() << change.getFilePath();

[ioeric]

• Return const refs in getXXX.

include/clang/Tooling/Refactoring/AtomicChange.h

59 ↗

(On Diff #90011)

I don't really have strong opinion about this. Another thing I was considering is whether the returned value always has an associated object in the class. For example, getError might return Error plus some extra message so that the return string can't be a reference. But these interfaces look pretty stable for now. And your examples make sense. Changed them to const ref instead.

[klimek]

lg

cfe/trunk/include/clang/Tooling/Refactoring/AtomicChange.h

+//===--- AtomicChange.h - AtomicChange class --------------------*- C++ -*-===//
+//
+//                     The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===----------------------------------------------------------------------===//
+//
+//  This file defines AtomicChange which is used to create a set of source
+//  changes, e.g. replacements and header insertions.
+//
+//===----------------------------------------------------------------------===//
+
+#ifndef LLVM_CLANG_TOOLING_REFACTOR_ATOMICCHANGE_H
+#define LLVM_CLANG_TOOLING_REFACTOR_ATOMICCHANGE_H
+
+#include "clang/Basic/SourceManager.h"
+#include "clang/Tooling/Core/Replacement.h"
+#include "llvm/ADT/StringRef.h"
+#include "llvm/Support/Error.h"
+
+namespace clang {
+namespace tooling {
+
+/// \brief An atomic change is used to create and group a set of source edits,
+/// e.g. replacements or header insertions. Edits in an AtomicChange should be
+/// related, e.g. replacements for the same type reference and the corresponding
+/// header insertion/deletion.
+///
+/// An AtomicChange is uniquely identified by a key and will either be fully
+/// applied or not applied at all.
+///
+/// Calling setError on an AtomicChange stores the error message and marks it as
+/// bad, i.e. none of its source edits will be applied.
+class AtomicChange {
+public:
+  /// \brief Creates an atomic change around \p KeyPosition with the key being a
+  /// concatenation of the file name and the offset of \p KeyPosition.
+  /// \p KeyPosition should be the location of the key syntactical element that
+  /// is being changed, e.g. the call to a refactored method.
+  AtomicChange(const SourceManager &SM, SourceLocation KeyPosition);
+
+  /// \brief Creates an atomic change for \p FilePath with a customized key.
+  AtomicChange(llvm::StringRef FilePath, llvm::StringRef Key)
+      : Key(Key), FilePath(FilePath) {}
+
+  /// \brief Returns the atomic change as a YAML string.
+  std::string toYAMLString();
+
+  /// \brief Converts a YAML-encoded automic change to AtomicChange.
+  static AtomicChange convertFromYAML(llvm::StringRef YAMLContent);
+
+  /// \brief Returns the key of this change, which is a concatenation of the
+  /// file name and offset of the key position.
+  const std::string &getKey() const { return Key; }
+
+  /// \brief Returns the path of the file containing this atomic change.
+  const std::string &getFilePath() const { return FilePath; }
+
+  /// \brief If this change could not be created successfully, e.g. because of
+  /// conflicts among replacements, use this to set an error description.
+  /// Thereby, places that cannot be fixed automatically can be gathered when
+  /// applying changes.
+  void setError(llvm::StringRef Error) { this->Error = Error; }
+
+  /// \brief Returns whether an error has been set on this list.
+  bool hasError() const { return !Error.empty(); }
+
+  /// \brief Returns the error message or an empty string if it does not exist.
+  const std::string &getError() const { return Error; }
+
+  /// \brief Adds a replacement that replaces range [Loc, Loc+Length) with
+  /// \p Text.
+  /// \returns An llvm::Error carrying ReplacementError on error.
+  llvm::Error replace(const SourceManager &SM, SourceLocation Loc,
+                      unsigned Length, llvm::StringRef Text);
+
+  /// \brief Adds a replacement that inserts \p Text at \p Loc. If this
+  /// insertion conflicts with an existing insertion (at the same position),
+  /// this will be inserted before/after the existing insertion depending on
+  /// \p InsertAfter. Users should use `replace` with `Length=0` instead if they
+  /// do not want conflict resolving by default. If the conflicting replacement
+  /// is not an insertion, an error is returned.
+  ///
+  /// \returns An llvm::Error carrying ReplacementError on error.
+  llvm::Error insert(const SourceManager &SM, SourceLocation Loc,
+                     llvm::StringRef Text, bool InsertAfter = true);
+
+  /// \brief Adds a header into the file that contains the key position.
+  /// Header can be in angle brackets or double quotation marks. By default
+  /// (header is not quoted), header will be surrounded with double quotes.
+  void addHeader(llvm::StringRef Header);
+
+  /// \brief Removes a header from the file that contains the key position.
+  void removeHeader(llvm::StringRef Header);
+
+  /// \brief Returns a const reference to existing replacements.
+  const Replacements &getReplacements() const { return Replaces; }
+
+  llvm::ArrayRef<std::string> getInsertedHeaders() const {
+    return InsertedHeaders;
+  }
+
+  llvm::ArrayRef<std::string> getRemovedHeaders() const {
+    return RemovedHeaders;
+  }
+
+private:
+  AtomicChange() {}
+
+  AtomicChange(std::string Key, std::string FilePath, std::string Error,
+               std::vector<std::string> InsertedHeaders,
+               std::vector<std::string> RemovedHeaders,
+               clang::tooling::Replacements Replaces);
+
+  // This uniquely identifies an AtomicChange.
+  std::string Key;
+  std::string FilePath;
+  std::string Error;
+  std::vector<std::string> InsertedHeaders;
+  std::vector<std::string> RemovedHeaders;
+  tooling::Replacements Replaces;
+};
+
+} // end namespace tooling
+} // end namespace clang
+
+#endif // LLVM_CLANG_TOOLING_REFACTOR_ATOMICCHANGE_H

cfe/trunk/lib/Tooling/CMakeLists.txt

 set(LLVM_LINK_COMPONENTS
   Option
   Support
   )
 
 add_subdirectory(Core)
+add_subdirectory(Refactoring)
 
 add_clang_library(clangTooling
   ArgumentsAdjusters.cpp
   CommonOptionsParser.cpp
   CompilationDatabase.cpp
   FileMatchTrie.cpp
   FixIt.cpp
   JSONCompilationDatabase.cpp

cfe/trunk/lib/Tooling/Refactoring/AtomicChange.cpp

+//===--- AtomicChange.cpp - AtomicChange implementation -----------------*- C++ -*-===//
+//
+//                     The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===----------------------------------------------------------------------===//
+
+#include "clang/Tooling/Refactoring/AtomicChange.h"
+#include "clang/Tooling/ReplacementsYaml.h"
+#include "llvm/Support/YAMLTraits.h"
+#include <string>
+
+LLVM_YAML_IS_FLOW_SEQUENCE_VECTOR(std::string)
+LLVM_YAML_IS_SEQUENCE_VECTOR(clang::tooling::AtomicChange)
+
+namespace {
+/// \brief Helper to (de)serialize an AtomicChange since we don't have direct
+/// access to its data members.
+/// Data members of a normalized AtomicChange can be directly mapped from/to
+/// YAML string.
+struct NormalizedAtomicChange {
+  NormalizedAtomicChange() = default;
+
+  NormalizedAtomicChange(const llvm::yaml::IO &) {}
+
+  // This converts AtomicChange's internal implementation of the replacements
+  // set to a vector of replacements.
+  NormalizedAtomicChange(const llvm::yaml::IO &,
+                         const clang::tooling::AtomicChange &E)
+      : Key(E.getKey()), FilePath(E.getFilePath()), Error(E.getError()),
+        InsertedHeaders(E.getInsertedHeaders()),
+        RemovedHeaders(E.getRemovedHeaders()),
+        Replaces(E.getReplacements().begin(), E.getReplacements().end()) {}
+
+  // This is not expected to be called but needed for template instantiation.
+  clang::tooling::AtomicChange denormalize(const llvm::yaml::IO &) {
+    llvm_unreachable("Do not convert YAML to AtomicChange directly with '>>'. "
+                     "Use AtomicChange::convertFromYAML instead.");
+  }
+  std::string Key;
+  std::string FilePath;
+  std::string Error;
+  std::vector<std::string> InsertedHeaders;
+  std::vector<std::string> RemovedHeaders;
+  std::vector<clang::tooling::Replacement> Replaces;
+};
+} // anonymous namespace
+
+namespace llvm {
+namespace yaml {
+
+/// \brief Specialized MappingTraits to describe how an AtomicChange is
+/// (de)serialized.
+template <> struct MappingTraits<NormalizedAtomicChange> {
+  static void mapping(IO &Io, NormalizedAtomicChange &Doc) {
+    Io.mapRequired("Key", Doc.Key);
+    Io.mapRequired("FilePath", Doc.FilePath);
+    Io.mapRequired("Error", Doc.Error);
+    Io.mapRequired("InsertedHeaders", Doc.InsertedHeaders);
+    Io.mapRequired("RemovedHeaders", Doc.RemovedHeaders);
+    Io.mapRequired("Replacements", Doc.Replaces);
+  }
+};
+
+/// \brief Specialized MappingTraits to describe how an AtomicChange is
+/// (de)serialized.
+template <> struct MappingTraits<clang::tooling::AtomicChange> {
+  static void mapping(IO &Io, clang::tooling::AtomicChange &Doc) {
+    MappingNormalization<NormalizedAtomicChange, clang::tooling::AtomicChange>
+        Keys(Io, Doc);
+    Io.mapRequired("Key", Keys->Key);
+    Io.mapRequired("FilePath", Keys->FilePath);
+    Io.mapRequired("Error", Keys->Error);
+    Io.mapRequired("InsertedHeaders", Keys->InsertedHeaders);
+    Io.mapRequired("RemovedHeaders", Keys->RemovedHeaders);
+    Io.mapRequired("Replacements", Keys->Replaces);
+  }
+};
+
+} // end namespace yaml
+} // end namespace llvm
+
+namespace clang {
+namespace tooling {
+
+AtomicChange::AtomicChange(const SourceManager &SM,
+                           SourceLocation KeyPosition) {
+  const FullSourceLoc FullKeyPosition(KeyPosition, SM);
+  std::pair<FileID, unsigned> FileIDAndOffset =
+      FullKeyPosition.getSpellingLoc().getDecomposedLoc();
+  const FileEntry *FE = SM.getFileEntryForID(FileIDAndOffset.first);
+  assert(FE && "Cannot create AtomicChange with invalid location.");
+  FilePath = FE->getName();
+  Key = FilePath + ":" + std::to_string(FileIDAndOffset.second);
+}
+
+AtomicChange::AtomicChange(std::string Key, std::string FilePath,
+                           std::string Error,
+                           std::vector<std::string> InsertedHeaders,
+                           std::vector<std::string> RemovedHeaders,
+                           clang::tooling::Replacements Replaces)
+    : Key(std::move(Key)), FilePath(std::move(FilePath)),
+      Error(std::move(Error)), InsertedHeaders(std::move(InsertedHeaders)),
+      RemovedHeaders(std::move(RemovedHeaders)), Replaces(std::move(Replaces)) {
+}
+
+std::string AtomicChange::toYAMLString() {
+  std::string YamlContent;
+  llvm::raw_string_ostream YamlContentStream(YamlContent);
+
+  llvm::yaml::Output YAML(YamlContentStream);
+  YAML << *this;
+  YamlContentStream.flush();
+  return YamlContent;
+}
+
+AtomicChange AtomicChange::convertFromYAML(llvm::StringRef YAMLContent) {
+  NormalizedAtomicChange NE;
+  llvm::yaml::Input YAML(YAMLContent);
+  YAML >> NE;
+  AtomicChange E(NE.Key, NE.FilePath, NE.Error, NE.InsertedHeaders,
+                 NE.RemovedHeaders, tooling::Replacements());
+  for (const auto &R : NE.Replaces) {
+    llvm::Error Err = E.Replaces.add(R);
+    if (Err)
+      llvm_unreachable(
+          "Failed to add replacement when Converting YAML to AtomicChange.");
+    llvm::consumeError(std::move(Err));
+  }
+  return E;
+}
+
+llvm::Error AtomicChange::insert(const SourceManager &SM, SourceLocation Loc,
+                                 llvm::StringRef Text, bool InsertAfter) {
+  if (Text.empty())
+    return llvm::Error::success();
+  Replacement R(SM, Loc, 0, Text);
+  llvm::Error Err = Replaces.add(R);
+  if (Err) {
+    return llvm::handleErrors(
+        std::move(Err), [&](const ReplacementError &RE) -> llvm::Error {
+          if (RE.get() != replacement_error::insert_conflict)
+            return llvm::make_error<ReplacementError>(RE);
+          unsigned NewOffset = Replaces.getShiftedCodePosition(R.getOffset());
+          if (!InsertAfter)
+            NewOffset -=
+                RE.getExistingReplacement()->getReplacementText().size();
+          Replacement NewR(R.getFilePath(), NewOffset, 0, Text);
+          Replaces = Replaces.merge(Replacements(NewR));
+          return llvm::Error::success();
+        });
+  }
+  return llvm::Error::success();
+}
+
+void AtomicChange::addHeader(llvm::StringRef Header) {
+  InsertedHeaders.push_back(Header);
+}
+
+void AtomicChange::removeHeader(llvm::StringRef Header) {
+  RemovedHeaders.push_back(Header);
+}
+
+} // end namespace tooling
+} // end namespace clang

cfe/trunk/lib/Tooling/Refactoring/CMakeLists.txt

+set(LLVM_LINK_COMPONENTS
+  Option
+  Support
+  )
+
+add_clang_library(clangToolingRefactor
+  AtomicChange.cpp
+
+  LINK_LIBS
+  clangBasic
+  clangToolingCore
+  )

cfe/trunk/unittests/Tooling/CMakeLists.txt

 set(LLVM_LINK_COMPONENTS
   ${LLVM_TARGETS_TO_BUILD}
   Support
   )
 
 # By default MSVC has a 2^16 limit on the number of sections in an object file,
 # and this needs more than that.
 if (MSVC)
   set_source_files_properties(RecursiveASTVisitorTest.cpp PROPERTIES COMPILE_FLAGS /bigobj)
   set_source_files_properties(RecursiveASTVisitorTestExprVisitor.cpp PROPERTIES COMPILE_FLAGS /bigobj)
 endif()
 
 add_clang_unittest(ToolingTests
   CommentHandlerTest.cpp
   CompilationDatabaseTest.cpp
-  FixItTest.cpp  
+  FixItTest.cpp
   LookupTest.cpp
   QualTypeNamesTest.cpp
   RecursiveASTVisitorTest.cpp
   RecursiveASTVisitorTestCallVisitor.cpp
   RecursiveASTVisitorTestDeclVisitor.cpp
   RecursiveASTVisitorTestExprVisitor.cpp
   RecursiveASTVisitorTestTypeLocVisitor.cpp
   RefactoringCallbacksTest.cpp
   RefactoringTest.cpp
   ReplacementsYamlTest.cpp
   RewriterTest.cpp
   ToolingTest.cpp
   )
 
 target_link_libraries(ToolingTests
   clangAST
   clangASTMatchers
   clangBasic
   clangFormat
   clangFrontend
   clangLex
   clangRewrite
   clangTooling
   clangToolingCore
+  clangToolingRefactor
   )

cfe/trunk/unittests/Tooling/RefactoringTest.cpp

 #include "clang/Basic/SourceManager.h"
 #include "clang/Basic/VirtualFileSystem.h"
 #include "clang/Format/Format.h"
 #include "clang/Frontend/CompilerInstance.h"
 #include "clang/Frontend/FrontendAction.h"
 #include "clang/Frontend/TextDiagnosticPrinter.h"
 #include "clang/Rewrite/Core/Rewriter.h"
 #include "clang/Tooling/Refactoring.h"
+#include "clang/Tooling/Refactoring/AtomicChange.h"
 #include "clang/Tooling/Tooling.h"
 #include "llvm/ADT/SmallString.h"
 #include "gtest/gtest.h"
 
 namespace clang {
 namespace tooling {
 
 TEST_F(ReplacementTest, CanDeleteAllText) {
TEST_F(ReplacementTest, ReturnsInvalidPath) {
   EXPECT_TRUE(Replace1.getFilePath().empty());
 
   Replacement Replace2;
   EXPECT_TRUE(Replace2.getFilePath().empty());
 }
 
 // Checks that an llvm::Error instance contains a ReplacementError with expected
 // error code, expected new replacement, and expected existing replacement.
-static bool checkReplacementError(
-    llvm::Error&& Error, replacement_error ExpectedErr,
+static bool checkReplacementError(llvm::Error &&Error,
+                                  replacement_error ExpectedErr,
                                   llvm::Optional<Replacement> ExpectedExisting,
                                   llvm::Optional<Replacement> ExpectedNew) {
   if (!Error) {
     llvm::errs() << "Error is a success.";
     return false;
   }
   std::string ErrorMessage;
   llvm::raw_string_ostream OS(ErrorMessage);
   llvm::handleAllErrors(std::move(Error), [&](const ReplacementError &RE) {
     llvm::errs() << "Handling error...\n";
#else
   StringRef Path2 = "a\\b\\c.h";
 #endif
   FileToReplaces[Path1] = Replacements();
   FileToReplaces[Path2] = Replacements();
   FileToReplaces = groupReplacementsByFile(FileMgr, FileToReplaces);
   EXPECT_TRUE(FileToReplaces.empty());
 }
 
+class AtomicChangeTest : public ::testing::Test {
+  protected:
+    void setUp() {
+      DefaultFileID = Context.createInMemoryFile("input.cpp", DefaultCode);
+      DefaultLoc = Context.Sources.getLocForStartOfFile(DefaultFileID)
+                       .getLocWithOffset(20);
+      assert(DefaultLoc.isValid() && "Default location must be valid.");
+    }
+
+    RewriterTestContext Context;
+    std::string DefaultCode = std::string(100, 'a');
+    unsigned DefaultOffset = 20;
+    SourceLocation DefaultLoc;
+    FileID DefaultFileID;
+};
+
+TEST_F(AtomicChangeTest, AtomicChangeToYAML) {
+  setUp();
+  AtomicChange Change(Context.Sources, DefaultLoc);
+  llvm::Error Err =
+      Change.insert(Context.Sources, DefaultLoc, "aa", /*InsertAfter=*/false);
+  ASSERT_TRUE(!Err);
+  Err = Change.insert(Context.Sources, DefaultLoc.getLocWithOffset(10), "bb",
+                    /*InsertAfter=*/false);
+  ASSERT_TRUE(!Err);
+  Change.addHeader("a.h");
+  Change.removeHeader("b.h");
+  std::string YAMLString = Change.toYAMLString();
+
+  // NOTE: If this test starts to fail for no obvious reason, check whitespace.
+  ASSERT_STREQ("---\n"
+               "Key:             'input.cpp:20'\n"
+               "FilePath:        input.cpp\n"
+               "Error:           ''\n"
+               "InsertedHeaders: [ a.h ]\n"
+               "RemovedHeaders:  [ b.h ]\n"
+               "Replacements:    \n" // Extra whitespace here!
+               "  - FilePath:        input.cpp\n"
+               "    Offset:          20\n"
+               "    Length:          0\n"
+               "    ReplacementText: aa\n"
+               "  - FilePath:        input.cpp\n"
+               "    Offset:          30\n"
+               "    Length:          0\n"
+               "    ReplacementText: bb\n"
+               "...\n",
+               YAMLString.c_str());
+}
+
+TEST_F(AtomicChangeTest, YAMLToAtomicChange) {
+  setUp();
+  std::string YamlContent = "---\n"
+                            "Key:             'input.cpp:20'\n"
+                            "FilePath:        input.cpp\n"
+                            "Error:           'ok'\n"
+                            "InsertedHeaders: [ a.h ]\n"
+                            "RemovedHeaders:  [ b.h ]\n"
+                            "Replacements:    \n" // Extra whitespace here!
+                            "  - FilePath:        input.cpp\n"
+                            "    Offset:          20\n"
+                            "    Length:          0\n"
+                            "    ReplacementText: aa\n"
+                            "  - FilePath:        input.cpp\n"
+                            "    Offset:          30\n"
+                            "    Length:          0\n"
+                            "    ReplacementText: bb\n"
+                            "...\n";
+  AtomicChange ExpectedChange(Context.Sources, DefaultLoc);
+  llvm::Error Err = ExpectedChange.insert(Context.Sources, DefaultLoc, "aa",
+                                        /*InsertAfter=*/false);
+  ASSERT_TRUE(!Err);
+  Err = ExpectedChange.insert(Context.Sources, DefaultLoc.getLocWithOffset(10),
+                            "bb", /*InsertAfter=*/false);
+  ASSERT_TRUE(!Err);
+
+  ExpectedChange.addHeader("a.h");
+  ExpectedChange.removeHeader("b.h");
+  ExpectedChange.setError("ok");
+
+  AtomicChange ActualChange = AtomicChange::convertFromYAML(YamlContent);
+  EXPECT_EQ(ExpectedChange.getKey(), ActualChange.getKey());
+  EXPECT_EQ(ExpectedChange.getFilePath(), ActualChange.getFilePath());
+  EXPECT_EQ(ExpectedChange.getError(), ActualChange.getError());
+  EXPECT_EQ(ExpectedChange.getInsertedHeaders(), ActualChange.getInsertedHeaders());
+  EXPECT_EQ(ExpectedChange.getRemovedHeaders(), ActualChange.getRemovedHeaders());
+  EXPECT_EQ(ExpectedChange.getReplacements().size(),
+            ActualChange.getReplacements().size());
+  EXPECT_EQ(2u, ActualChange.getReplacements().size());
+  EXPECT_EQ(*ExpectedChange.getReplacements().begin(),
+            *ActualChange.getReplacements().begin());
+  EXPECT_EQ(*(++ExpectedChange.getReplacements().begin()),
+            *(++ActualChange.getReplacements().begin()));
+}
+
+TEST_F(AtomicChangeTest, CheckKeyAndKeyFile) {
+  setUp();
+  AtomicChange Change(Context.Sources, DefaultLoc);
+  EXPECT_EQ("input.cpp:20", Change.getKey());
+  EXPECT_EQ("input.cpp", Change.getFilePath());
+}
+
+TEST_F(AtomicChangeTest, InsertBefore) {
+  setUp();
+  AtomicChange Change(Context.Sources, DefaultLoc);
+  llvm::Error Err = Change.insert(Context.Sources, DefaultLoc, "aa");
+  ASSERT_TRUE(!Err);
+  EXPECT_EQ(Change.getReplacements().size(), 1u);
+  EXPECT_EQ(*Change.getReplacements().begin(),
+            Replacement(Context.Sources, DefaultLoc, 0, "aa"));
+  Err = Change.insert(Context.Sources, DefaultLoc, "b", /*InsertAfter=*/false);
+  ASSERT_TRUE(!Err);
+  EXPECT_EQ(Change.getReplacements().size(), 1u);
+  EXPECT_EQ(*Change.getReplacements().begin(),
+            Replacement(Context.Sources, DefaultLoc, 0, "baa"));
+}
+
+TEST_F(AtomicChangeTest, InsertAfter) {
+  setUp();
+  AtomicChange Change(Context.Sources, DefaultLoc);
+  llvm::Error Err = Change.insert(Context.Sources, DefaultLoc, "aa");
+  ASSERT_TRUE(!Err);
+  EXPECT_EQ(Change.getReplacements().size(), 1u);
+  EXPECT_EQ(*Change.getReplacements().begin(),
+            Replacement(Context.Sources, DefaultLoc, 0, "aa"));
+  Err = Change.insert(Context.Sources, DefaultLoc, "b");
+  ASSERT_TRUE(!Err);
+  EXPECT_EQ(Change.getReplacements().size(), 1u);
+  EXPECT_EQ(*Change.getReplacements().begin(),
+            Replacement(Context.Sources, DefaultLoc, 0, "aab"));
+}
+
+TEST_F(AtomicChangeTest, InsertBeforeWithInvalidLocation) {
+  setUp();
+  AtomicChange Change(Context.Sources, DefaultLoc);
+  llvm::Error Err =
+      Change.insert(Context.Sources, DefaultLoc, "a", /*InsertAfter=*/false);
+  ASSERT_TRUE(!Err);
+
+  // Invalid location.
+  Err = Change.insert(Context.Sources, SourceLocation(), "a",
+                    /*InsertAfter=*/false);
+  ASSERT_TRUE((bool)Err);
+  EXPECT_TRUE(checkReplacementError(
+      std::move(Err), replacement_error::wrong_file_path,
+      Replacement(Context.Sources, DefaultLoc, 0, "a"),
+      Replacement(Context.Sources, SourceLocation(), 0, "a")));
+
+}
+
+TEST_F(AtomicChangeTest, InsertBeforeToWrongFile) {
+  setUp();
+  AtomicChange Change(Context.Sources, DefaultLoc);
+  llvm::Error Err =
+      Change.insert(Context.Sources, DefaultLoc, "a", /*InsertAfter=*/false);
+  ASSERT_TRUE(!Err);
+
+  // Inserting at a different file.
+  FileID NewID = Context.createInMemoryFile("extra.cpp", DefaultCode);
+  SourceLocation NewLoc = Context.Sources.getLocForStartOfFile(NewID);
+  Err = Change.insert(Context.Sources, NewLoc, "b", /*InsertAfter=*/false);
+  ASSERT_TRUE((bool)Err);
+  EXPECT_TRUE(
+      checkReplacementError(std::move(Err), replacement_error::wrong_file_path,
+                            Replacement(Context.Sources, DefaultLoc, 0, "a"),
+                            Replacement(Context.Sources, NewLoc, 0, "b")));
+}
+
+TEST_F(AtomicChangeTest, InsertAfterWithInvalidLocation) {
+  setUp();
+  AtomicChange Change(Context.Sources, DefaultLoc);
+  llvm::Error Err = Change.insert(Context.Sources, DefaultLoc, "a");
+  ASSERT_TRUE(!Err);
+
+  // Invalid location.
+  Err = Change.insert(Context.Sources, SourceLocation(), "b");
+  ASSERT_TRUE((bool)Err);
+  EXPECT_TRUE(checkReplacementError(
+      std::move(Err), replacement_error::wrong_file_path,
+      Replacement(Context.Sources, DefaultLoc, 0, "a"),
+      Replacement(Context.Sources, SourceLocation(), 0, "b")));
+}
+
 } // end namespace tooling
 } // end namespace clang