[PATCH] D27054: Introducing clang::tooling::AtomicChange for refactoring tools.

[Eric Liu via Phabricator via cfe-commits]

ioeric added inline comments.


================
Comment at: include/clang/Tooling/Refactoring/AtomicChange.h:59
+  /// \brief Returns the path of the file containing this atomic change.
+  std::string getFilePath() const { return FilePath; }
+
----------------
alexshap wrote:
> ioeric wrote:
> > alexshap wrote:
> > > i assume i might be missing smth - why here and above (in getKey, getFilePath, getError) std::string is returned by value  ?
> > Otherwise, users would need to worry about the lifetimes of the object. And these methods are not expected to be called intensively, so performance is not an issue.
> i see, thanks for the explanation, probably not particularly important
> i thought that if a caller wanted to get a copy / take ownership 
> it would just **accept** it by value, i.e. 
>    auto path = change.getFilePath() /* assuming getFilePath returns const ref */ 
> At the same time if for example we need to check the filepath (or just print it),
> not copies would be required:
>    llvm::errs() << change.getFilePath();
I don't really have strong opinion about this. Another thing I was considering is whether the returned value always has an associated object in the class. For example, `getError` might return `Error` plus some extra message so that the return string can't be a reference. But these interfaces look pretty stable for now. And your examples make sense. Changed them to const ref instead.


https://reviews.llvm.org/D27054