[cfe-commits] Refactoring Tool patch submission

[Thomas Minor]

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.

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/20120610/d6c9694d/attachment.html>