[cfe-commits] Refactoring Tool patch submission
klimek at google.com
Sun Jun 10 23:25:19 PDT 2012
On Sun, Jun 10, 2012 at 8:33 PM, Thomas Minor <sxeraverx at gmail.com> wrote:
> On Sun, Jun 10, 2012 at 11:24 AM, Manuel Klimek <klimek at google.com> wrote:
>> On Sun, Jun 10, 2012 at 5:47 PM, Thomas Minor <sxeraverx at gmail.com>wrote:
>>> This certainly isn't the final solution. However, I think it's better
>>> than applying them in alphabetical order. In this case, restricting them to
>>> emission order is something that gives you more control.
>> My problem with this is that it gives you unwanted coupling. Once users
>> of the library rely on the order it's something that needs to be supported.
>> Fair enough. Undefined behavior is undefined.
>> My main concern is still that I don't see how what you propose this
>> should work with multiple TUs, where for example a header file is parsed by
>> different TUs.
>> I suppose the idea was that when a header is processed as a part of
> multiple translation units (or even multiple times within the same
> translation unit), the places where it has already been edited will show up
> as invalid, and the tool would know not to touch those. I still have to
> think about it some more to get the concept fully fleshed out.
I'd rather figure out a more principled approach now than start having
users rely on ordering.
Here's a proposal: when adding replacements, we could also give a
sequencing number, with the constraint that replacements will be applied in
strict sequencing order; multiple replacements with the same sequencing
constraint may be applied in arbitrary order.
>> The main problem seems to be with the concept of a SourceLocation, when
>> interacting with the rewriter--on a deletion, a whole range of
>> SourceLocations get mapped to a physical location, and on insertion, a
>> single SourceLocation gets mapped to a whole range of physical locations.
>> They should be invalidated whenever any source that included them or was
>> adjacent to them was rewritten. If the rewriter returned new virtual
>> locations corresponding to its insertions (invalidate 1, generate 2),
>> replacements (invalidate many, generate 2), and deletions (invalidate many,
>> generate 1), it would be much easier to specify the source ranges of
>> interest, and would get rid of the need to sequence edits at all, as well
>> as the whole InsertText/InsertTextAfter/InsertTextBefore ordeal.
>> A drawback of this method is that a tool that uses the Rewriter would
>> have to go and patch the AST itself, to update source locations of
>> replaced/inserted nodes and their neighbors. There doesn't seem to be an
>> automated way of doing this.
>> For non-overlapping edits, reverse source order isn't even necessary--as
>> it is, the Rewriter takes care of updating source locations.
>> On Sun, Jun 10, 2012 at 3:08 AM, Manuel Klimek <klimek at google.com> wrote:
>>> On Sun, Jun 10, 2012 at 12:52 AM, Thomas Minor <sxeraverx at gmail.com>wrote:
>>>> I would like to submit a patch to the Refactoring Tool. Currently, the
>>>> tool relies on std::set for uniqueing edits. However, this poses problems
>>>> for insertions (in the form of replacements of a zero-length range): they
>>>> can be reordered arbitrarily. This patch causes them to be applied in the
>>>> order submitted to the tool.
>>> I'm not sure this is the right solution - I don't want to rely on the
>>> tool emitting edits in the right order, because I don't see a way this can
>>> be guaranteed when running over multiple translation units.
>>> For non-overlapping edits, ordering is easy to achieve after-the-fact by
>>> applying in reverse source order.
>>> For overlapping edits, the edits would need to contain information on
>>> how they are sequenced in a TU independent way; I don't think the sequence
>>> in which they are emitted is a good sequence, as it restricts the tools too
-------------- next part --------------
An HTML attachment was scrubbed...
More information about the cfe-commits