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

[Manuel Klimek]

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