If you like I can also go through and point out all the places where I
*removed* copies.

On Wed, Sep 21, 2016 at 5:14 PM Zachary Turner <ztur...@google.com> wrote:

> On Wed, Sep 21, 2016 at 5:00 PM Greg Clayton <gclay...@apple.com> wrote:
> Please submit a change requests when doing these kinds of things and let
> people comment on the changes before committing such things.
> We deleted functions that were correctly using "const char *" like:
>   bool Execute(llvm::StringRef string, Match *match = nullptr) const;
>   bool Execute(const char *, Match * = nullptr) = delete;
> Yet inside these functions you do "string.str().c_str()" because these
> need to be null terminated? this indicates that StringRef is NOT the
> correct choice here. You will make heap copy of all strings (if they are
> long enough and regex matching string can be very long) that you compile or
> execute. We already make a copy of the string when we compile, so StringRef
> didn't really save us anything on compile and execute is always making a
> copy just to run the expression on. I am fine with both being there in case
> one might be more efficient, but taking them out just to use a less
> efficient version that uses llvm::StringRef is not the kind of changes we
> should be doing all over.
> We will make copies of all strings in all of the following changes:
> - Unneeded copy:
> -        new TypeNameSpecifierImpl(regex->GetText(), true));
> +        new TypeNameSpecifierImpl(regex->GetText().str().c_str(), true));
> All of these copies are only temporary.  As I've said over and over, once
> everything is StringRef all the way down, the copies all disappear.  It's
> only a copy because TypeNameSpecifierImpl doesn't take a STringRef, and I
> didn't want to change the whole codebase all at once, but rather
> incrementally.
lldb-commits mailing list

Reply via email to