[Lldb-commits] [lldb] r282079 - Make lldb::Regex use StringRef.

[Greg Clayton via lldb-commits]

> On Sep 21, 2016, at 5:14 PM, Zachary Turner <zturner at google.com> wrote:
> 
> 
> 
> On Wed, Sep 21, 2016 at 5:00 PM Greg Clayton <gclayton at 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.