> On 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.

As part of these changes we absolutely should add StringRef to these classes 
and we should just make it part of the changes. It know these strings are 
temporary, I am not worried about memory, I am worried about efficiency of the 
heap copy of the string that temporarily is created during this call. If things 
were taking "const char *" before, copies were not being made. If they were 
being made, then they were probably being made for good reason. I would love to 
see the places where all of these copies were removed, but if they were "const 
char *" there were no copies going in, unless they needed to. If they didn't 
need to make the copies, then we can fix that function.

lldb-commits mailing list

Reply via email to