[Rewrite] Extend to further accept CharSourceRange

[jdenny]

Summary

Some Rewrite functions are already overloaded to accept CharSourceRange, and this extends others in the same manner. I'm calling these in code that's not ready to upstream, but I figure they might be useful to others in the meantime.

This patch also adds some unit tests. I'm not very familiar with unit testing here. I used clangTooling to build the AST. Is that appropriate?

[jdenny]

Ping. This seems like a straight-forward extension to the Rewriter API.

[jdenny]

clang/unittests/Rewrite/RewriterTest.cpp

2

Oops. Need to change the description here.

[jdenny]

Ping.

[jdenny]

Ping.

[jdenny]

Rebased, fixed a comment, and applied a clang-format suggestion.

Ping.

[jdoerfert]

Could you describe the unit tests a little, what is tested and why.

[jdenny]

@jdoerfert Thanks for your reply. I added some comments to the tests. Let me know whether that's what you're looking for.

[jdoerfert]

The comments help a lot. LGTM.

[thakis]

cfe/trunk/unittests/Rewrite/CMakeLists.txt

12 ↗

(On Diff #208250)

This makes RewriteTests depend on clangTooling, and in follow-ups on clangFrontend and clangSerialization. Rewrite used to depend on basically only clangBasic and clangLex, and now it depends on almost all of clang. Maybe there's a less heavy-weight way to test this?

Also, when do you expect to land code that uses this? Checking in code that's unused upstream over a long period of time seems suboptimal. I delete code that shows up unused on http://llvm-cs.pcc.me.uk/ every now and then for example. (Ignore this part if you expect to check in clients of the overload soon.)

[jdenny]

cfe/trunk/unittests/Rewrite/CMakeLists.txt

12 ↗

(On Diff #208250)

This makes RewriteTests depend on clangTooling, and in follow-ups on clangFrontend and clangSerialization. Rewrite used to depend on basically only clangBasic and clangLex, and now it depends on almost all of clang. Maybe there's a less heavy-weight way to test this?

Sure, I can look for another way to test this. I didn't realize this would be a real problem. But....

Also, when do you expect to land code that uses this?

Not soon. I offered this patch thinking that obvious Rewrite extensions like this one could prove useful to the larger community even if there are no upstream users now. I'm also fine to revert and keep my Rewrite extensions private for now. Is that the best approach?

[jdenny]

cfe/trunk/unittests/Rewrite/CMakeLists.txt

12 ↗

(On Diff #208250)

This makes RewriteTests depend on clangTooling, and in follow-ups on clangFrontend and clangSerialization. Rewrite used to depend on basically only clangBasic and clangLex, and now it depends on almost all of clang. Maybe there's a less heavy-weight way to test this?

I'm thinking of adding more Rewrite unit tests in the future. Does anyone have advice on this issue? That is, is there any easy way to build ASTs in unit tests without such dependencies? On the other hand, these dependencies are common throughout clang/unittests subdirectories, so I'm not sure why RewriteTests needs to avoid them.

clang/include/clang/Rewrite/Core/Rewriter.h

public:
   int getRangeSize(const CharSourceRange &Range,
                    RewriteOptions opts = RewriteOptions()) const;
 
   /// getRewrittenText - Return the rewritten form of the text in the specified
   /// range.  If the start or end of the range was unrewritable or if they are
   /// in different buffers, this returns an empty string.
   ///
   /// Note that this method is not particularly efficient.
-  std::string getRewrittenText(SourceRange Range) const;
+  std::string getRewrittenText(CharSourceRange Range) const;
+
+  /// getRewrittenText - Return the rewritten form of the text in the specified
+  /// range.  If the start or end of the range was unrewritable or if they are
+  /// in different buffers, this returns an empty string.
+  ///
+  /// Note that this method is not particularly efficient.
+  std::string getRewrittenText(SourceRange Range) const {
+    return getRewrittenText(CharSourceRange::getTokenRange(Range));
+  }
 
   /// InsertText - Insert the specified string at the specified location in the
   /// original buffer.  This method returns true (and does nothing) if the input
   /// location was not rewritable, false otherwise.
   ///
   /// \param indentNewLines if true new lines in the string are indented
   /// using the indentation of the source line in position \p Loc.
   bool InsertText(SourceLocation Loc, StringRef Str,
public:
   /// buffer with a new string.  This is effectively a combined "remove/insert"
   /// operation.
   bool ReplaceText(SourceLocation Start, unsigned OrigLength,
                    StringRef NewStr);
 
   /// ReplaceText - This method replaces a range of characters in the input
   /// buffer with a new string.  This is effectively a combined "remove/insert"
   /// operation.
+  bool ReplaceText(CharSourceRange range, StringRef NewStr) {
+    return ReplaceText(range.getBegin(), getRangeSize(range), NewStr);
+  }
+
+  /// ReplaceText - This method replaces a range of characters in the input
+  /// buffer with a new string.  This is effectively a combined "remove/insert"
+  /// operation.
   bool ReplaceText(SourceRange range, StringRef NewStr) {
     return ReplaceText(range.getBegin(), getRangeSize(range), NewStr);
   }
 
   /// ReplaceText - This method replaces a range of characters in the input
   /// buffer with a new string.  This is effectively a combined "remove/insert"
   /// operation.
   bool ReplaceText(SourceRange range, SourceRange replacementRange);

clang/lib/Rewrite/Rewriter.cpp

int Rewriter::getRangeSize(SourceRange Range, RewriteOptions opts) const {
   return getRangeSize(CharSourceRange::getTokenRange(Range), opts);
 }
 
 /// getRewrittenText - Return the rewritten form of the text in the specified
 /// range.  If the start or end of the range was unrewritable or if they are
 /// in different buffers, this returns an empty string.
 ///
 /// Note that this method is not particularly efficient.
-std::string Rewriter::getRewrittenText(SourceRange Range) const {
+std::string Rewriter::getRewrittenText(CharSourceRange Range) const {
   if (!isRewritable(Range.getBegin()) ||
       !isRewritable(Range.getEnd()))
     return {};
 
   FileID StartFileID, EndFileID;
   unsigned StartOff, EndOff;
   StartOff = getLocationOffsetAndFileID(Range.getBegin(), StartFileID);
   EndOff   = getLocationOffsetAndFileID(Range.getEnd(), EndFileID);
 
   if (StartFileID != EndFileID)
     return {}; // Start and end in different buffers.
 
   // If edits have been made to this buffer, the delta between the range may
   // have changed.
   std::map<FileID, RewriteBuffer>::const_iterator I =
     RewriteBuffers.find(StartFileID);
   if (I == RewriteBuffers.end()) {
     // If the buffer hasn't been rewritten, just return the text from the input.
     const char *Ptr = SourceMgr->getCharacterData(Range.getBegin());
 
     // Adjust the end offset to the end of the last token, instead of being the
     // start of the last token.
-    EndOff += Lexer::MeasureTokenLength(Range.getEnd(), *SourceMgr, *LangOpts);
+    if (Range.isTokenRange())
+      EndOff +=
+          Lexer::MeasureTokenLength(Range.getEnd(), *SourceMgr, *LangOpts);
     return std::string(Ptr, Ptr+EndOff-StartOff);
   }
 
   const RewriteBuffer &RB = I->second;
   EndOff = RB.getMappedOffset(EndOff, true);
   StartOff = RB.getMappedOffset(StartOff);
 
   // Adjust the end offset to the end of the last token, instead of being the
   // start of the last token.
+  if (Range.isTokenRange())
     EndOff += Lexer::MeasureTokenLength(Range.getEnd(), *SourceMgr, *LangOpts);
 
   // Advance the iterators to the right spot, yay for linear time algorithms.
   RewriteBuffer::iterator Start = RB.begin();
   std::advance(Start, StartOff);
   RewriteBuffer::iterator End = Start;
   std::advance(End, EndOff-StartOff);
 
   return std::string(Start, End);

clang/unittests/Rewrite/CMakeLists.txt

 set(LLVM_LINK_COMPONENTS
   Support
   )
 
 add_clang_unittest(RewriteTests
   RewriteBufferTest.cpp
+  RewriterTest.cpp
   )
 target_link_libraries(RewriteTests
   PRIVATE
   clangRewrite
+  clangTooling
   )

clang/unittests/Rewrite/RewriterTest.cpp

+//===- unittests/Rewrite/RewriterTest.cpp - Rewriter tests ----------------===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+
+#include "clang/Rewrite/Core/Rewriter.h"
+#include "clang/Tooling/Tooling.h"
+#include "gtest/gtest.h"
+
+using namespace clang;
+
+namespace {
+
+struct RangeTypeTest {
+  std::unique_ptr<ASTUnit> AST;
+  Rewriter Rewrite;
+  SourceLocation FileStart;
+  CharSourceRange CRange;
+  CharSourceRange TRange;
+  SourceRange SRange;
+  SourceLocation makeLoc(int Off) { return FileStart.getLocWithOffset(Off); }
+  CharSourceRange makeCharRange(int StartOff, int EndOff) {
+    return CharSourceRange::getCharRange(makeLoc(StartOff), makeLoc(EndOff));
+  }
+  RangeTypeTest(StringRef Code, int StartOff, int EndOff) {
+    AST = tooling::buildASTFromCode(Code);
+    ASTContext &C = AST->getASTContext();
+    Rewrite = Rewriter(C.getSourceManager(), C.getLangOpts());
+    FileStart = AST->getStartOfMainFileID();
+    CRange = makeCharRange(StartOff, EndOff);
+    SRange = CRange.getAsRange();
+    TRange = CharSourceRange::getTokenRange(SRange);
+  }
+};
+
+TEST(Rewriter, GetRewrittenTextRangeTypes) {
+  StringRef Code = "int main() { return 0; }";
+  //                             ^~~+++
+  RangeTypeTest T(Code, 13, 16);
+  EXPECT_EQ(T.Rewrite.getRewrittenText(T.CRange), "ret");
+  EXPECT_EQ(T.Rewrite.getRewrittenText(T.TRange), "return");
+  EXPECT_EQ(T.Rewrite.getRewrittenText(T.SRange), "return");
+  T.Rewrite.InsertText(T.makeLoc(13), "x");
+  EXPECT_EQ(T.Rewrite.getRewrittenText(T.CRange), "xret");
+  EXPECT_EQ(T.Rewrite.getRewrittenText(T.TRange), "xreturn");
+  EXPECT_EQ(T.Rewrite.getRewrittenText(T.SRange), "xreturn");
+}
+
+TEST(Rewriter, ReplaceTextRangeTypes) {
+  StringRef Code = "int main(int argc, char *argv[]) { return argc; }";
+  //                                                          ^~++
+  //                                                          ^~~~~
+  RangeTypeTest T(Code, 42, 44);
+  T.Rewrite.ReplaceText(T.CRange, "foo");
+  EXPECT_EQ(T.Rewrite.getRewrittenText(T.makeCharRange(42, 47)), "foogc;");
+  T.Rewrite.ReplaceText(T.TRange, "bar");
+  EXPECT_EQ(T.Rewrite.getRewrittenText(T.makeCharRange(42, 47)), "bar;");
+  T.Rewrite.ReplaceText(T.SRange, "0");
+  EXPECT_EQ(T.Rewrite.getRewrittenText(T.makeCharRange(42, 47)), "0;");
+}
+
+} // anonymous namespace