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

[Zachary Turner via lldb-commits]

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.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/lldb-commits/attachments/20160922/b828a66e/attachment.html>