On Sun, Jun 10, 2012 at 8:33 PM, Thomas Minor <[email protected]> wrote:
> On Sun, Jun 10, 2012 at 11:24 AM, Manuel Klimek <[email protected]> wrote: > >> On Sun, Jun 10, 2012 at 5:47 PM, Thomas Minor <[email protected]>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 <[email protected]> wrote: >> >>> On Sun, Jun 10, 2012 at 12:52 AM, Thomas Minor <[email protected]>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 >>> >>> >> >> >
_______________________________________________ cfe-commits mailing list [email protected] http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
