Implements refactoring support for multiple TUs in one process.

[chandlerc, 2011-06-02 00:34:28]

Initial comments on interfaces...

http://codereview.appspot.com/4526098/diff/1/include/clang/Tooling/Refactoring.h
File include/clang/Tooling/Refactoring.h (right):

http://codereview.appspot.com/4526098/diff/1/include/clang/Tooling/Refactorin...
include/clang/Tooling/Refactoring.h:44: /// \brief Creates a replacement of the
range [Offset, Offset+Length[ in File
the second '[' should probably be a ']'

http://codereview.appspot.com/4526098/diff/1/include/clang/Tooling/Refactorin...
include/clang/Tooling/Refactoring.h:47: llvm::StringRef File, unsigned Offset,
File is an ambiguous name here... FilePath? What form of path? (relative?
absolute? maybe it doesn't matter..)

http://codereview.appspot.com/4526098/diff/1/include/clang/Tooling/Refactorin...
include/clang/Tooling/Refactoring.h:49: bool operator<(const Replacement &Other)
const;
Comment on why this exists? This doesn't seem like an intuitively sortable
construct to me.

If its for the de-duping below, I'd generally prefer a comparison functor
explicit given in that context.

http://codereview.appspot.com/4526098/diff/1/include/clang/Tooling/Refactorin...
include/clang/Tooling/Refactoring.h:54: /// \brief Creates a Replacement of the
range [Start, Start+Length[ with
Why wouldn't this accept a SourceRange? (also, same typo as above)

http://codereview.appspot.com/4526098/diff/1/include/clang/Tooling/Refactorin...
include/clang/Tooling/Refactoring.h:57: SourceManager &Sources, SourceLocation
Start, unsigned Length,
Sources? Not SM?

Also, why a static function instead of a constructor?

http://codereview.appspot.com/4526098/diff/1/include/clang/Tooling/Refactorin...
include/clang/Tooling/Refactoring.h:67: /// \brief A set of replacments.
What is the advantage of this over just using std::set?

http://codereview.appspot.com/4526098/diff/1/include/clang/Tooling/Refactorin...
include/clang/Tooling/Refactoring.h:75: /// \brief Add a replacement of an AST
node's text with ReplacementText.
Why isn't this a templated constructor on Replacement?

http://codereview.appspot.com/4526098/diff/1/include/clang/Tooling/Refactorin...
include/clang/Tooling/Refactoring.h:94: int getRangeSize(SourceManager &Sources,
const CharSourceRange &Range);
Why can't this be a private static function in either Replacements or
Replacement? That would constrain its scope...

http://codereview.appspot.com/4526098/diff/1/include/clang/Tooling/Refactorin...
include/clang/Tooling/Refactoring.h:108: /// This is a refactoring specific
version of \see ClangTool.
Why doesn't it share an interface?

http://codereview.appspot.com/4526098/diff/1/include/clang/Tooling/Tooling.h
File include/clang/Tooling/Tooling.h (right):

http://codereview.appspot.com/4526098/diff/1/include/clang/Tooling/Tooling.h#...
include/clang/Tooling/Tooling.h:99: /// This class is written to be usable for
command line utilities.
I think this comment needs updating to reflect the dual usage patterns. One is
driven through an interface for querying a database of commandlines, the other
by parsing a single compile-like commandline. We should be very clear about
that.

http://codereview.appspot.com/4526098/diff/1/include/clang/Tooling/Tooling.h#...
include/clang/Tooling/Tooling.h:106: /// RunCommandLines.
s/RunCommandLines/RunCommandLine/

http://codereview.appspot.com/4526098/diff/1/include/clang/Tooling/Tooling.h#...
include/clang/Tooling/Tooling.h:109: /// \brief Construct a clang tool from a
command line.
Is this redundant? It should be the same as an empty tool + RunCommandLine()?

At the very least it would be good to update the documentation for this to
clarify how it relates to the other modes of operation.

http://codereview.appspot.com/4526098/diff/1/include/clang/Tooling/Tooling.h#...
include/clang/Tooling/Tooling.h:143: /// Translation units not found in
JsonDatabaseDirectory will be skipped.
WTF is JsonDatabaseDirectory? An opaque argument that is just a stringref needs
*some* link to its documentation...

http://codereview.appspot.com/4526098/diff/1/include/clang/Tooling/Tooling.h#...
include/clang/Tooling/Tooling.h:146: const std::vector<std::string>
&TranslationUnits);
ArrayRef for TranslationUnites?

http://codereview.appspot.com/4526098/diff/1/include/clang/Tooling/Tooling.h#...
include/clang/Tooling/Tooling.h:156: const std::vector<std::string> &
CommandLine, FrontendAction *ToolAction);
no space between & and CommandLine. Also should use ArrayRef here.

[klimek, 2011-06-04 04:22:53]

http://codereview.appspot.com/4526098/diff/1/include/clang/Tooling/Refactoring.h
File include/clang/Tooling/Refactoring.h (right):

http://codereview.appspot.com/4526098/diff/1/include/clang/Tooling/Refactorin...
include/clang/Tooling/Refactoring.h:44: /// \brief Creates a replacement of the
range [Offset, Offset+Length[ in File
On 2011/06/02 00:34:28, chandlerc wrote:
> the second '[' should probably be a ']'

Yay European vs. American teachings. Done.

http://codereview.appspot.com/4526098/diff/1/include/clang/Tooling/Refactorin...
include/clang/Tooling/Refactoring.h:47: llvm::StringRef File, unsigned Offset,
On 2011/06/02 00:34:28, chandlerc wrote:
> File is an ambiguous name here... FilePath? What form of path? (relative?
> absolute? maybe it doesn't matter..)

Added comment.

http://codereview.appspot.com/4526098/diff/1/include/clang/Tooling/Refactorin...
include/clang/Tooling/Refactoring.h:49: bool operator<(const Replacement &Other)
const;
On 2011/06/02 00:34:28, chandlerc wrote:
> Comment on why this exists? This doesn't seem like an intuitively sortable
> construct to me.
> 
> If its for the de-duping below, I'd generally prefer a comparison functor
> explicit given in that context.

Put into a Less Comparator with a comment.

http://codereview.appspot.com/4526098/diff/1/include/clang/Tooling/Refactorin...
include/clang/Tooling/Refactoring.h:54: /// \brief Creates a Replacement of the
range [Start, Start+Length[ with
On 2011/06/02 00:34:28, chandlerc wrote:
> Why wouldn't this accept a SourceRange? (also, same typo as above)

Added a constructor from CharSourceRange. We still want the other one for the
use case when writing tools.

http://codereview.appspot.com/4526098/diff/1/include/clang/Tooling/Refactorin...
include/clang/Tooling/Refactoring.h:57: SourceManager &Sources, SourceLocation
Start, unsigned Length,
On 2011/06/02 00:34:28, chandlerc wrote:
> Sources? Not SM?
> 
> Also, why a static function instead of a constructor?

I really stumble every time I read SM.

http://codereview.appspot.com/4526098/diff/1/include/clang/Tooling/Refactorin...
include/clang/Tooling/Refactoring.h:67: /// \brief A set of replacments.
On 2011/06/02 00:34:28, chandlerc wrote:
> What is the advantage of this over just using std::set?

Done.

http://codereview.appspot.com/4526098/diff/1/include/clang/Tooling/Refactorin...
include/clang/Tooling/Refactoring.h:75: /// \brief Add a replacement of an AST
node's text with ReplacementText.
On 2011/06/02 00:34:28, chandlerc wrote:
> Why isn't this a templated constructor on Replacement?

Done.

http://codereview.appspot.com/4526098/diff/1/include/clang/Tooling/Refactorin...
include/clang/Tooling/Refactoring.h:94: int getRangeSize(SourceManager &Sources,
const CharSourceRange &Range);
On 2011/06/02 00:34:28, chandlerc wrote:
> Why can't this be a private static function in either Replacements or
> Replacement? That would constrain its scope...

The problem is that I don't think it has anything lost there ... I want to move
that into the source manager once this code is in and we actually know we want
this, but I don't want this code review to be blocked on that change.

http://codereview.appspot.com/4526098/diff/1/include/clang/Tooling/Refactorin...
include/clang/Tooling/Refactoring.h:108: /// This is a refactoring specific
version of \see ClangTool.
On 2011/06/02 00:34:28, chandlerc wrote:
> Why doesn't it share an interface?

Currently because I have not yet figure out the use cases on that level. Until I
have figured that out, I'd like to use as little inheritance as possible in
order keep changes easy.
I added a FIXME and would like to address that in a follow-up change.

http://codereview.appspot.com/4526098/diff/1/include/clang/Tooling/Tooling.h
File include/clang/Tooling/Tooling.h (right):

http://codereview.appspot.com/4526098/diff/1/include/clang/Tooling/Tooling.h#...
include/clang/Tooling/Tooling.h:99: /// This class is written to be usable for
command line utilities.
On 2011/06/02 00:34:28, chandlerc wrote:
> I think this comment needs updating to reflect the dual usage patterns. One is
> driven through an interface for querying a database of commandlines, the other
> by parsing a single compile-like commandline. We should be very clear about
> that.

Split up.

http://codereview.appspot.com/4526098/diff/1/include/clang/Tooling/Tooling.h#...
include/clang/Tooling/Tooling.h:106: /// RunCommandLines.
On 2011/06/02 00:34:28, chandlerc wrote:
> s/RunCommandLines/RunCommandLine/

Removed constructor.

http://codereview.appspot.com/4526098/diff/1/include/clang/Tooling/Tooling.h#...
include/clang/Tooling/Tooling.h:109: /// \brief Construct a clang tool from a
command line.
On 2011/06/02 00:34:28, chandlerc wrote:
> Is this redundant? It should be the same as an empty tool + RunCommandLine()?
> 
> At the very least it would be good to update the documentation for this to
> clarify how it relates to the other modes of operation.

Removed the other modes.

http://codereview.appspot.com/4526098/diff/1/include/clang/Tooling/Tooling.h#...
include/clang/Tooling/Tooling.h:143: /// Translation units not found in
JsonDatabaseDirectory will be skipped.
On 2011/06/02 00:34:28, chandlerc wrote:
> WTF is JsonDatabaseDirectory? An opaque argument that is just a stringref
needs
> *some* link to its documentation...

This is private now, and I added a node -> constructor.

http://codereview.appspot.com/4526098/diff/1/include/clang/Tooling/Tooling.h#...
include/clang/Tooling/Tooling.h:146: const std::vector<std::string>
&TranslationUnits);
On 2011/06/02 00:34:28, chandlerc wrote:
> ArrayRef for TranslationUnites?

Done.

http://codereview.appspot.com/4526098/diff/1/include/clang/Tooling/Tooling.h#...
include/clang/Tooling/Tooling.h:156: const std::vector<std::string> &
CommandLine, FrontendAction *ToolAction);
On 2011/06/02 00:34:28, chandlerc wrote:
> no space between & and CommandLine. Also should use ArrayRef here.
 
Removed.