[PATCH] cp11-migrate: Integration with LibFormat.

Guillaume Papin guillaume.papin at epitech.eu
Mon Jul 22 13:01:40 PDT 2013



================
Comment at: cpp11-migrate/Core/FileOverrides.cpp:104
@@ +103,3 @@
+
+  for (Replacements::iterator I = Replaces.begin(), E = Replaces.end(); I != E;
+       ++I) {
----------------
Edwin Vane wrote:
> Perhaps a comment saying you're grouping Replacements by file name.
Sure. Done.

================
Comment at: cpp11-migrate/Core/FileOverrides.cpp:207
@@ +206,3 @@
+    // replacement inside range -> resize the range
+    if (Range.contains(Replace)) {
+      int Difference = ReplaceNewSize - Replace.getLength();
----------------
Edwin Vane wrote:
> Why change the range at all? If a replacement is completely within an existing range, the part of the range outside of the replacement is still something that has changed right?
Yes you are right it is changed. What I'm doing here is resizing the range so it takes the replacement into account.

Let's say we have a range: `Range(/*offset=*/0, /*length=*/5)`
And a replacement: `Replacement(/*offset=*/2, /*length=*/1, /*replacement-text=*/"~~~")`

The replacement deletes 1 character but adds 3 new characters, a difference of +2 characters.
That's why I'm resizing the range to take the new characters into account, final range is `Range(0, 7)`.

================
Comment at: cpp11-migrate/Core/FileOverrides.cpp:224
@@ +223,3 @@
+
+  const tooling::Range Replace;
+  const unsigned ReplaceNewSize;
----------------
Edwin Vane wrote:
> Can't you just use const lvalue ref here instead of constructing a new Range?
A range is just 2 integers, it's cheaper to pass by value.


http://llvm-reviews.chandlerc.com/D1136



More information about the cfe-commits mailing list