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

[Douglas Gregor]

On May 3, 2012, at 11:10 AM, 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.

This is the kind of thing Argyrios knows all about :) CC'd.

> 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?

It's fine to keep it as an implementation detail while working out the details.

	- Doug