Add `replace` interface with range in AtomicChange.

[ioeric]

Please see comment from a previous patch (https://reviews.llvm.org/D27054?id=79099#inline-234270). Generally, AtomicChange is a higher level of abstraction than Replacement, and we don't want users to deal with Replacement and the related errors.

[hokein]

Get rid of Replacement, and implement "replace" interface with range.

[alexander-shaposhnikov]

to avoid misunderstanding - are the tools like clang-rename, change-namespace and clang-reorder-fields supposed to use AtomicChange (via the methods insert, replace etc) ?
(i am asking just to make sure i understand correctly the intentions behind AtomicChange interface)

[ioeric]

In D31492#713908, @alexshap wrote:

to avoid misunderstanding - are the tools like clang-rename, change-namespace and clang-reorder-fields supposed to use AtomicChange (via the methods insert, replace etc) ?
(i am asking just to make sure i understand correctly the intentions behind AtomicChange interface)

In the future, yes. We have a plan to redesign ClangTool interface and the refactoring infrastructure (e.g. to support multi-TU/codebase-wide refactoring) which would use AtomicChange interface in the user level.

[hokein]

Rebase to master.

[ioeric]

Could you add a simple test?

[hokein]

Add unittest.

[ioeric]

Looks good and thanks for the cleanup.

[hokein]

are the tools like clang-rename, change-namespace and clang-reorder-fields supposed to use AtomicChange (via the methods insert, replace etc) ?

Yeah. We are using AtomicChange in clang-rename refinement (D31176).

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

public:
   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 the given Range with
+  /// ReplacementText.
+  /// \returns An llvm::Error carrying ReplacementError on error.
+  llvm::Error replace(const SourceManager &SM, const CharSourceRange &Range,
+                      llvm::StringRef ReplacementText);
+
   /// \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),

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

for (const auto &R : NE.Replaces) {
     if (Err)
       llvm_unreachable(
           "Failed to add replacement when Converting YAML to AtomicChange.");
     llvm::consumeError(std::move(Err));
   }
   return E;
 }
 
+llvm::Error AtomicChange::replace(const SourceManager &SM,
+                                  const CharSourceRange &Range,
+                                  llvm::StringRef ReplacementText) {
+  return Replaces.add(Replacement(SM, Range, ReplacementText));
+}
+
 llvm::Error AtomicChange::replace(const SourceManager &SM, SourceLocation Loc,
                                   unsigned Length, llvm::StringRef Text) {
   return Replaces.add(Replacement(SM, Loc, Length, Text));
 }
 
 llvm::Error AtomicChange::insert(const SourceManager &SM, SourceLocation Loc,
                                  llvm::StringRef Text, bool InsertAfter) {
   if (Text.empty())

cfe/trunk/unittests/Tooling/RefactoringTest.cpp

#endif
   FileToReplaces[Path1] = Replacements();
   FileToReplaces[Path2] = Replacements();
   FileToReplaces = groupReplacementsByFile(FileMgr, FileToReplaces);
   EXPECT_TRUE(FileToReplaces.empty());
 }
 
 class AtomicChangeTest : public ::testing::Test {
   protected:
-    void setUp() {
+    void SetUp() override {
       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");
ASSERT_STREQ("---\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"
TEST_F(AtomicChangeTest, YAMLToAtomicChange) {
   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, Replace) {
-  setUp();
   AtomicChange Change(Context.Sources, DefaultLoc);
   llvm::Error Err = Change.replace(Context.Sources, DefaultLoc, 2, "aa");
   ASSERT_TRUE(!Err);
   EXPECT_EQ(Change.getReplacements().size(), 1u);
   EXPECT_EQ(*Change.getReplacements().begin(),
             Replacement(Context.Sources, DefaultLoc, 2, "aa"));
 
   // Add a new replacement that conflicts with the existing one.
   Err = Change.replace(Context.Sources, DefaultLoc, 3, "ab");
   EXPECT_TRUE((bool)Err);
   llvm::consumeError(std::move(Err));
   EXPECT_EQ(Change.getReplacements().size(), 1u);
 }
 
+TEST_F(AtomicChangeTest, ReplaceWithRange) {
+  AtomicChange Change(Context.Sources, DefaultLoc);
+  SourceLocation End = DefaultLoc.getLocWithOffset(20);
+  llvm::Error Err = Change.replace(
+      Context.Sources, CharSourceRange::getCharRange(DefaultLoc, End), "aa");
+  ASSERT_TRUE(!Err);
+  EXPECT_EQ(Change.getReplacements().size(), 1u);
+  EXPECT_EQ(*Change.getReplacements().begin(),
+            Replacement(Context.Sources, DefaultLoc, 20, "aa"));
+}
+
 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