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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
