[cfe-commits] [PATCH] X-TU Refactoring support

[Manuel Klimek]

+djasper

On Thu, May 3, 2012 at 8:10 PM, Manuel Klimek <klimek at google.com> wrote:
> On Fri, Apr 27, 2012 at 7:49 PM, Douglas Gregor <dgregor at apple.com> wrote:
>> On Apr 19, 2012, at 4:30 AM, Manuel Klimek <klimek at google.com> wrote:
>>
>>> The attached patch adds a layer on top of the tooling support for
>>> cross-TU refactorings.
>>
>>
>> +static const char * const InvalidLocation = "invalid-location";
>> +
>> +Replacement::Replacement()
>> +  : FilePath(InvalidLocation), Offset(0), Length(0) {}
>> +
>>
>> If we have to pick a sentinel, can it at least be an empty FilePath?
>>
>> +bool saveRewrittenFiles(Rewriter &Rewrite) {
>> +  for (Rewriter::buffer_iterator I = Rewrite.buffer_begin(),
>> +                                 E = Rewrite.buffer_end();
>> +       I != E; ++I) {
>> +    // FIXME: This code is copied from the FixItRewriter.cpp - I think it should
>> +    // go into directly into Rewriter (there we also have the Diagnostics to
>> +    // handle the error cases better).
>> +    const FileEntry *Entry =
>> +        Rewrite.getSourceMgr().getFileEntryForID(I->first);
>> +    std::string ErrorInfo;
>> +    llvm::raw_fd_ostream FileStream(
>> +        Entry->getName(), ErrorInfo, llvm::raw_fd_ostream::F_Binary);
>> +    if (!ErrorInfo.empty())
>> +      return false;
>> +    I->second.write(FileStream);
>> +    FileStream.flush();
>> +  }
>> +  return true;
>> +}
>>
>> I agree that this should go into the Rewriter. Go for it :)
>>
>> +int getRangeSize(SourceManager &Sources, const CharSourceRange &Range) {
>> +  SourceLocation SpellingBegin = Sources.getSpellingLoc(Range.getBegin());
>> +  SourceLocation SpellingEnd = Sources.getSpellingLoc(Range.getEnd());
>> +  std::pair<FileID, unsigned> Start = Sources.getDecomposedLoc(SpellingBegin);
>> +  std::pair<FileID, unsigned> End = Sources.getDecomposedLoc(SpellingEnd);
>> +  if (Start.first != End.first) return -1;
>> +  if (Range.isTokenRange())
>> +    // FIXME: Bring in the correct LangOptions.
>> +    End.second += Lexer::MeasureTokenLength(SpellingEnd, Sources,
>> +                                            LangOptions());
>> +  return End.second - Start.second;
>> +}
>>
>> This should probably return an 'unsigned'. It belongs in the Lexer, IMO.
>
> So return bool and use and unsigned reference to return the value?
>
> My main concern with moving that right now is that we recently
> discovered that this might not be the right solution yet to create a
> replacement from an AST node - we'll probably want to drill through
> macros and find the right point inside the expansion closest to the
> spelling where the full node is expressed.
> I've looked at makeFileCharRange in the Lexer, which seemed related at
> a first glance, but I have no idea what it really does :)
>
> My problem is currently that I could need some ideas on what to do
> here, as the general case seems really hard to implement and I'm a
> little lost.
>
> On the other hand, I think using the spelling location is a good first
> approximation (as it actually works most of the time), so I think this
> is a good first step, and I don't think keeping the implementation of
> how to figure out what range is covered by a node a detail of the
> refactoring lib is a big problem right now.
>
> Thoughts?
>
>> I'm not sure whether you've looked at the Edit library or not, but its functionality fits well with what you're doing here, because it can order rewrites such that a given set of rewrite operations is more likely to succeed. For example, if one rewrite edits part of a function body and another rewrite removes that function entirely, the edit library will deal with it whereas the rewriter by itself could fail. There's no specific action item here, other than a suggestion that there's some potential goodness in combining these features.
>
> I've looked and found that it didn't completely fit what we're trying
> to do with the refactoring library (mainly creating intermediate
> representations that are not depending on a sourcemanager to then
> apply them later in bulk over different TUs). I'll take a deeper look
> to figure out how we might get some of the benefits you mentioned out
> of it.
>
> Thanks for the review!
> /Manuel