[cfe-commits] Refactoring Tool patch submission

[Manuel Klimek]

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.

Thoughts?
/Manuel


>
> Cheers,
> -Thomas
>
>
>>
>> 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.
>>
>> Cheers,
>> -Thomas
>>
>>
>> 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:
>>>
>>>> Hello,
>>>>
>>>> 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
>>> much.
>>>
>>> Thoughts?
>>> /Manuel
>>>
>>>
>>
>>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20120611/9cabbc3c/attachment.html>