[PATCH] D27054: Introducing clang::tooling::EditList for refactoring tools.
ioeric updated this revision to Diff 90011.
ioeric added a comment.
- Addressed review comments.
- Make key customize-able.
- Added replace() and insert() to replace current replacement interfaces.
- s/EditList/AtomicChange/
https://reviews.llvm.org/D27054
Files:
include/clang/Tooling/Refactoring/AtomicChange.h
lib/Tooling/CMakeLists.txt
lib/Tooling/Refactoring/AtomicChange.cpp
lib/Tooling/Refactoring/CMakeLists.txt
unittests/Tooling/CMakeLists.txt
unittests/Tooling/RefactoringTest.cpp
Index: unittests/Tooling/RefactoringTest.cpp
===
--- unittests/Tooling/RefactoringTest.cpp
+++ unittests/Tooling/RefactoringTest.cpp
@@ -26,6 +26,7 @@
#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"
@@ -102,10 +103,10 @@
// 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,
-llvm::Optional ExpectedExisting,
-llvm::Optional ExpectedNew) {
+static bool checkReplacementError(llvm::Error &,
+ replacement_error ExpectedErr,
+ llvm::Optional ExpectedExisting,
+ llvm::Optional ExpectedNew) {
if (!Error) {
llvm::errs() << "Error is a success.";
return false;
@@ -1089,5 +1090,187 @@
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);
+
+
[PATCH] D27054: Introducing clang::tooling::EditList for refactoring tools.
ioeric updated this revision to Diff 79736.
ioeric added a comment.
- Added replace() and insert() to replace current replacement interfaces.
https://reviews.llvm.org/D27054
Files:
include/clang/Tooling/Refactoring/EditList.h
lib/Tooling/CMakeLists.txt
lib/Tooling/Refactoring/CMakeLists.txt
lib/Tooling/Refactoring/EditList.cpp
unittests/Tooling/CMakeLists.txt
unittests/Tooling/RefactoringTest.cpp
Index: unittests/Tooling/RefactoringTest.cpp
===
--- unittests/Tooling/RefactoringTest.cpp
+++ unittests/Tooling/RefactoringTest.cpp
@@ -26,6 +26,7 @@
#include "clang/Frontend/TextDiagnosticPrinter.h"
#include "clang/Rewrite/Core/Rewriter.h"
#include "clang/Tooling/Refactoring.h"
+#include "clang/Tooling/Refactoring/EditList.h"
#include "clang/Tooling/Tooling.h"
#include "llvm/ADT/SmallString.h"
#include "gtest/gtest.h"
@@ -102,10 +103,10 @@
// 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,
-llvm::Optional ExpectedExisting,
-llvm::Optional ExpectedNew) {
+static bool checkReplacementError(llvm::Error &,
+ replacement_error ExpectedErr,
+ llvm::Optional ExpectedExisting,
+ llvm::Optional ExpectedNew) {
if (!Error) {
llvm::errs() << "Error is a success.";
return false;
@@ -1089,5 +1090,187 @@
EXPECT_TRUE(FileToReplaces.empty());
}
+class EditListTest : 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(EditListTest, EditListToYAML) {
+ setUp();
+ EditList Edit(Context.Sources, DefaultLoc);
+ llvm::Error Err =
+ Edit.insert(Context.Sources, DefaultLoc, "aa", /*InsertAfter=*/false);
+ ASSERT_TRUE(!Err);
+ Err = Edit.insert(Context.Sources, DefaultLoc.getLocWithOffset(10), "bb",
+/*InsertAfter=*/false);
+ ASSERT_TRUE(!Err);
+ Edit.addHeader("a.h");
+ Edit.removeHeader("b.h");
+ std::string YAMLString = Edit.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(EditListTest, YAMLToEditList) {
+ 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";
+ EditList ExpectedEdit(Context.Sources, DefaultLoc);
+ llvm::Error Err = ExpectedEdit.insert(Context.Sources, DefaultLoc, "aa",
+/*InsertAfter=*/false);
+ ASSERT_TRUE(!Err);
+ Err = ExpectedEdit.insert(Context.Sources, DefaultLoc.getLocWithOffset(10),
+"bb", /*InsertAfter=*/false);
+ ASSERT_TRUE(!Err);
+
+ ExpectedEdit.addHeader("a.h");
+ ExpectedEdit.removeHeader("b.h");
+ ExpectedEdit.setError("ok");
+
+ EditList ActualEdit =
[PATCH] D27054: Introducing clang::tooling::EditList for refactoring tools.
djasper added inline comments. Comment at: include/clang/Tooling/Refactoring/EditList.h:82-94 + /// \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 the existing insertion. If the conflicting + /// replacement is not an insertion, an error is returned. + /// + /// \returns An llvm::Error carrying ReplacementError on error. + llvm::Error insertBefore(const SourceManager , SourceLocation Loc, ioeric wrote: > djasper wrote: > > ioeric wrote: > > > djasper wrote: > > > > Do we currently actually need these functions? They seem a bit > > > > dangerous. > > > 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. > > 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. > `replace` + `insert` sounds like a good way to go. Do we want to support > resolving general conflicts besides insertions though? 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. https://reviews.llvm.org/D27054 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D27054: Introducing clang::tooling::EditList for refactoring tools.
ioeric added inline comments. Comment at: include/clang/Tooling/Refactoring/EditList.h:82-94 + /// \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 the existing insertion. If the conflicting + /// replacement is not an insertion, an error is returned. + /// + /// \returns An llvm::Error carrying ReplacementError on error. + llvm::Error insertBefore(const SourceManager , SourceLocation Loc, djasper wrote: > ioeric wrote: > > djasper wrote: > > > Do we currently actually need these functions? They seem a bit dangerous. > > 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. > 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. `replace` + `insert` sounds like a good way to go. Do we want to support resolving general conflicts besides insertions though? https://reviews.llvm.org/D27054 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D27054: Introducing clang::tooling::EditList for refactoring tools.
djasper added inline comments.
Comment at: include/clang/Tooling/Refactoring/EditList.h:41
+ /// \brief Creates an edit list for a key position.
+ EditList(const SourceManager , SourceLocation KeyPosition);
+
ioeric wrote:
> ioeric wrote:
> > klimek wrote:
> > > ioeric wrote:
> > > > djasper wrote:
> > > > > 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 , SourceLocation
> > > > > KeyPosition);
> > > > 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.
> > > 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?).
> > 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!
> 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`.
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.
Comment at: include/clang/Tooling/Refactoring/EditList.h:82-94
+ /// \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 the existing insertion. If the conflicting
+ /// replacement is not an insertion, an error is returned.
+ ///
+ /// \returns An llvm::Error carrying ReplacementError on error.
+ llvm::Error insertBefore(const SourceManager , SourceLocation Loc,
ioeric wrote:
> djasper wrote:
> > Do we currently actually need these functions? They seem a bit dangerous.
> 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.
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.
Comment at: lib/Tooling/Refactoring/EditList.cpp:164
+
+llvm::Error EditList::insertAfter(const SourceManager , SourceLocation Loc,
+ llvm::StringRef Text) {
Don't duplicate this code. Add an internal function that takes an extra
parameter that controls conflict resolution.
https://reviews.llvm.org/D27054
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D27054: Introducing clang::tooling::EditList for refactoring tools.
ioeric added inline comments. Comment at: include/clang/Tooling/Refactoring/EditList.h:41 + /// \brief Creates an edit list for a key position. + EditList(const SourceManager , SourceLocation KeyPosition); + ioeric wrote: > klimek wrote: > > ioeric wrote: > > > djasper wrote: > > > > 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 , SourceLocation > > > > KeyPosition); > > > 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. > > 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?). > 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! 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`. https://reviews.llvm.org/D27054 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D27054: Introducing clang::tooling::EditList for refactoring tools.
ioeric updated this revision to Diff 79728.
ioeric added a comment.
- Make key customize-able.
https://reviews.llvm.org/D27054
Files:
include/clang/Tooling/Refactoring/EditList.h
lib/Tooling/CMakeLists.txt
lib/Tooling/Refactoring/CMakeLists.txt
lib/Tooling/Refactoring/EditList.cpp
unittests/Tooling/CMakeLists.txt
unittests/Tooling/RefactoringTest.cpp
Index: unittests/Tooling/RefactoringTest.cpp
===
--- unittests/Tooling/RefactoringTest.cpp
+++ unittests/Tooling/RefactoringTest.cpp
@@ -26,6 +26,7 @@
#include "clang/Frontend/TextDiagnosticPrinter.h"
#include "clang/Rewrite/Core/Rewriter.h"
#include "clang/Tooling/Refactoring.h"
+#include "clang/Tooling/Refactoring/EditList.h"
#include "clang/Tooling/Tooling.h"
#include "llvm/ADT/SmallString.h"
#include "gtest/gtest.h"
@@ -102,10 +103,10 @@
// 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,
-llvm::Optional ExpectedExisting,
-llvm::Optional ExpectedNew) {
+static bool checkReplacementError(llvm::Error &,
+ replacement_error ExpectedErr,
+ llvm::Optional ExpectedExisting,
+ llvm::Optional ExpectedNew) {
if (!Error) {
llvm::errs() << "Error is a success.";
return false;
@@ -1089,5 +1090,183 @@
EXPECT_TRUE(FileToReplaces.empty());
}
+class EditListTest : 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(EditListTest, EditListToYAML) {
+ setUp();
+ EditList Edit(Context.Sources, DefaultLoc);
+ llvm::Error Err = Edit.insertBefore(Context.Sources, DefaultLoc, "aa");
+ ASSERT_TRUE(!Err);
+ Err =
+ Edit.insertBefore(Context.Sources, DefaultLoc.getLocWithOffset(10), "bb");
+ ASSERT_TRUE(!Err);
+ Edit.addHeader("a.h");
+ Edit.removeHeader("b.h");
+ std::string YAMLString = Edit.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(EditListTest, YAMLToEditList) {
+ 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";
+ EditList ExpectedEdit(Context.Sources, DefaultLoc);
+ llvm::Error Err =
+ ExpectedEdit.insertBefore(Context.Sources, DefaultLoc, "aa");
+ ASSERT_TRUE(!Err);
+ Err = ExpectedEdit.insertBefore(Context.Sources,
+ DefaultLoc.getLocWithOffset(10), "bb");
+ ASSERT_TRUE(!Err);
+
+ ExpectedEdit.addHeader("a.h");
+ ExpectedEdit.removeHeader("b.h");
+ ExpectedEdit.setError("ok");
+
+ EditList ActualEdit = EditList::convertFromYAML(YamlContent);
+ EXPECT_EQ(ExpectedEdit.getKey(), ActualEdit.getKey());
+ EXPECT_EQ(ExpectedEdit.getFilePath(), ActualEdit.getFilePath());
+
[PATCH] D27054: Introducing clang::tooling::EditList for refactoring tools.
ioeric added inline comments. Comment at: include/clang/Tooling/Refactoring/EditList.h:41 + /// \brief Creates an edit list for a key position. + EditList(const SourceManager , SourceLocation KeyPosition); + klimek wrote: > ioeric wrote: > > djasper wrote: > > > 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 , SourceLocation > > > KeyPosition); > > 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. > 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?). 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! https://reviews.llvm.org/D27054 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D27054: Introducing clang::tooling::EditList for refactoring tools.
ioeric added inline comments. Comment at: include/clang/Tooling/Refactoring/EditList.h:41 + /// \brief Creates an edit list for a key position. + EditList(const SourceManager , SourceLocation KeyPosition); + djasper wrote: > 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 , SourceLocation > KeyPosition); 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. Comment at: include/clang/Tooling/Refactoring/EditList.h:43 + + EditList(std::string Key, std::string FilePath, std::string Error, + std::vector InsertedHeaders, djasper wrote: > 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? You are right! Make this private now. Comment at: include/clang/Tooling/Refactoring/EditList.h:82-94 + /// \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 the existing insertion. If the conflicting + /// replacement is not an insertion, an error is returned. + /// + /// \returns An llvm::Error carrying ReplacementError on error. + llvm::Error insertBefore(const SourceManager , SourceLocation Loc, djasper wrote: > Do we currently actually need these functions? They seem a bit dangerous. 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. https://reviews.llvm.org/D27054 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D27054: Introducing clang::tooling::EditList for refactoring tools.
ioeric updated this revision to Diff 79716.
ioeric marked 4 inline comments as done.
ioeric added a comment.
- Addressed review comments.
https://reviews.llvm.org/D27054
Files:
include/clang/Tooling/Refactoring/EditList.h
lib/Tooling/CMakeLists.txt
lib/Tooling/Refactoring/CMakeLists.txt
lib/Tooling/Refactoring/EditList.cpp
unittests/Tooling/CMakeLists.txt
unittests/Tooling/RefactoringTest.cpp
Index: unittests/Tooling/RefactoringTest.cpp
===
--- unittests/Tooling/RefactoringTest.cpp
+++ unittests/Tooling/RefactoringTest.cpp
@@ -26,6 +26,7 @@
#include "clang/Frontend/TextDiagnosticPrinter.h"
#include "clang/Rewrite/Core/Rewriter.h"
#include "clang/Tooling/Refactoring.h"
+#include "clang/Tooling/Refactoring/EditList.h"
#include "clang/Tooling/Tooling.h"
#include "llvm/ADT/SmallString.h"
#include "gtest/gtest.h"
@@ -102,10 +103,10 @@
// 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,
-llvm::Optional ExpectedExisting,
-llvm::Optional ExpectedNew) {
+static bool checkReplacementError(llvm::Error &,
+ replacement_error ExpectedErr,
+ llvm::Optional ExpectedExisting,
+ llvm::Optional ExpectedNew) {
if (!Error) {
llvm::errs() << "Error is a success.";
return false;
@@ -1089,5 +1090,183 @@
EXPECT_TRUE(FileToReplaces.empty());
}
+class EditListTest : 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(EditListTest, EditListToYAML) {
+ setUp();
+ EditList Edit(Context.Sources, DefaultLoc);
+ llvm::Error Err = Edit.insertBefore(Context.Sources, DefaultLoc, "aa");
+ ASSERT_TRUE(!Err);
+ Err =
+ Edit.insertBefore(Context.Sources, DefaultLoc.getLocWithOffset(10), "bb");
+ ASSERT_TRUE(!Err);
+ Edit.addHeader("a.h");
+ Edit.removeHeader("b.h");
+ std::string YAMLString = Edit.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(EditListTest, YAMLToEditList) {
+ 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";
+ EditList ExpectedEdit(Context.Sources, DefaultLoc);
+ llvm::Error Err =
+ ExpectedEdit.insertBefore(Context.Sources, DefaultLoc, "aa");
+ ASSERT_TRUE(!Err);
+ Err = ExpectedEdit.insertBefore(Context.Sources,
+ DefaultLoc.getLocWithOffset(10), "bb");
+ ASSERT_TRUE(!Err);
+
+ ExpectedEdit.addHeader("a.h");
+ ExpectedEdit.removeHeader("b.h");
+ ExpectedEdit.setError("ok");
+
+ EditList ActualEdit = EditList::convertFromYAML(YamlContent);
+ EXPECT_EQ(ExpectedEdit.getKey(), ActualEdit.getKey());
+
[PATCH] D27054: Introducing clang::tooling::EditList for refactoring tools.
djasper added inline comments.
Comment at: include/clang/Tooling/Refactoring/EditList.h:41
+ /// \brief Creates an edit list for a key position.
+ EditList(const SourceManager , SourceLocation KeyPosition);
+
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 , SourceLocation KeyPosition);
Comment at: include/clang/Tooling/Refactoring/EditList.h:43
+
+ EditList(std::string Key, std::string FilePath, std::string Error,
+ std::vector InsertedHeaders,
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?
Comment at: include/clang/Tooling/Refactoring/EditList.h:82-94
+ /// \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 the existing insertion. If the conflicting
+ /// replacement is not an insertion, an error is returned.
+ ///
+ /// \returns An llvm::Error carrying ReplacementError on error.
+ llvm::Error insertBefore(const SourceManager , SourceLocation Loc,
Do we currently actually need these functions? They seem a bit dangerous.
Comment at: include/clang/Tooling/Refactoring/EditList.h:117
+
+ std::string Key;
+ std::string FilePath;
Maybe add a short description of what "Key" contains. The others are pretty
intuitive.
Comment at: lib/Tooling/Refactoring/EditList.cpp:25
+ /// access to its data members.
+ struct NormalizedEditList {
+NormalizedEditList(const IO &) {}
I don't understand (without more comments) why this is normalized/denormalized.
Comment at: unittests/Tooling/RefactoringTest.cpp:1113
+ llvm::Error Err = Edit.insertBefore(Context.Sources, DefaultLoc, "aa");
+ EXPECT_TRUE(!Err);
+ Err =
I think these need to be ASSERT_TRUE.
https://reviews.llvm.org/D27054
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D27054: Introducing clang::tooling::EditList for refactoring tools.
ioeric added a comment. Ping. Daniel, could you take a look? Manuel and I would like your opinion on this. https://reviews.llvm.org/D27054 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D27054: Introducing clang::tooling::EditList for refactoring tools.
ioeric created this revision.
ioeric added a reviewer: klimek.
ioeric added subscribers: djasper, cfe-commits.
Herald added a subscriber: mgorny.
An edit list is used to create and group a set of source edits, e.g.
replacements or header insertions. Edits in an EditList should be related,
e.g. replacements for the same type reference and the corresponding header
insertion/deletion.
An EditList 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.
https://reviews.llvm.org/D27054
Files:
include/clang/Tooling/Refactoring/EditList.h
lib/Tooling/CMakeLists.txt
lib/Tooling/Refactoring/CMakeLists.txt
lib/Tooling/Refactoring/EditList.cpp
unittests/Tooling/CMakeLists.txt
unittests/Tooling/RefactoringTest.cpp
Index: unittests/Tooling/RefactoringTest.cpp
===
--- unittests/Tooling/RefactoringTest.cpp
+++ unittests/Tooling/RefactoringTest.cpp
@@ -26,6 +26,7 @@
#include "clang/Frontend/TextDiagnosticPrinter.h"
#include "clang/Rewrite/Core/Rewriter.h"
#include "clang/Tooling/Refactoring.h"
+#include "clang/Tooling/Refactoring/EditList.h"
#include "clang/Tooling/Tooling.h"
#include "llvm/ADT/SmallString.h"
#include "gtest/gtest.h"
@@ -102,10 +103,10 @@
// 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,
-llvm::Optional ExpectedExisting,
-llvm::Optional ExpectedNew) {
+static bool checkReplacementError(llvm::Error &,
+ replacement_error ExpectedErr,
+ llvm::Optional ExpectedExisting,
+ llvm::Optional ExpectedNew) {
if (!Error) {
llvm::errs() << "Error is a success.";
return false;
@@ -1089,5 +1090,183 @@
EXPECT_TRUE(FileToReplaces.empty());
}
+class EditListTest : 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(EditListTest, EditListToYAML) {
+ setUp();
+ EditList Edit(Context.Sources, DefaultLoc);
+ llvm::Error Err = Edit.insertBefore(Context.Sources, DefaultLoc, "aa");
+ EXPECT_TRUE(!Err);
+ Err =
+ Edit.insertBefore(Context.Sources, DefaultLoc.getLocWithOffset(10), "bb");
+ EXPECT_TRUE(!Err);
+ Edit.addHeader("a.h");
+ Edit.removeHeader("b.h");
+ std::string YAMLString = Edit.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(EditListTest, YAMLToEditList) {
+ 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";
+ EditList ExpectedEdit(Context.Sources, DefaultLoc);
+ llvm::Error Err =
+
