[Lldb-commits] [lldb] r282090 - Fix failing regex tests.

[Zachary Turner via lldb-commits]

Looks like llvm's regex is better than LLDB's in this regard, since it
supports explicitly setting the end pointer.  I can see a couple options:

1) Check if it's null terminated by peeking one past the end, and copying
if it's not.  This is pretty hackish, not crazy about this idea.
2) Un-delete the const char * version of the function but leave the
StringRef overload, find all places where I added the explicit conversion
and remove them so they invoke the const char* overload.
3) Change lldb::RegularExpression to just delegate to llvm under the hood
and set the end pointer.

On Wed, Sep 21, 2016 at 4:44 PM Zachary Turner <zturner at google.com> wrote:

> Actually wait, it doesn't.  It just explicitly sets the end pointer.
>
> On Wed, Sep 21, 2016 at 4:44 PM Zachary Turner <zturner at google.com> wrote:
>
> Worth noting that llvm::Regex has this constructor:
>
>
> Regex::Regex(StringRef regex, unsigned Flags) {
>   unsigned flags = 0;
>   preg = new llvm_regex();
>   preg->re_endp = regex.end();
>   if (Flags & IgnoreCase)
>     flags |= REG_ICASE;
>   if (Flags & Newline)
>     flags |= REG_NEWLINE;
>   if (!(Flags & BasicRegex))
>     flags |= REG_EXTENDED;
>   error = llvm_regcomp(preg, regex.data(), flags|REG_PEND);
> }
>
> So it assumes null termination even though you have a StringRef.
>
> On Wed, Sep 21, 2016 at 4:43 PM Zachary Turner <zturner at google.com> wrote:
>
> You need to duplicate something on the heap once when you execute the
> regex.  And in turn you save tens or hundreds or copies on the way there
> because of inefficient string usage.
>
> We could also just un-delete the overload that takes a const char*, then
> the duplication would only ever happen when you explicitly use a StringRef.
>
> I don't agree this should be reverted.  In the process of doing this
> conversion I eliminated numerous careless string copies.
>
> On Wed, Sep 21, 2016 at 4:38 PM Greg Clayton <gclayton at apple.com> wrote:
>
> This it the perfect example of why not to use a StringRef since the string
> needs to be null terminated. Why did we change this? Now even if you call
> this function:
>
> RegularExpression r(...);
>
> r.Execute(".......................", ...)
>
> You will need to duplicate the string on the heap just to execute this.
> Please revert this. Anything that requires null terminate is not a
> candidate for converting to StringRef.
>
>
> > On Sep 21, 2016, at 10:13 AM, Zachary Turner via lldb-commits <
> lldb-commits at lists.llvm.org> wrote:
> >
> > Author: zturner
> > Date: Wed Sep 21 12:13:51 2016
> > New Revision: 282090
> >
> > URL: http://llvm.org/viewvc/llvm-project?rev=282090&view=rev
> > Log:
> > Fix failing regex tests.
> >
> > r282079 converted the regular expression interface to accept
> > and return StringRefs instead of char pointers.  In one case
> > a null pointer check was converted to an empty string check,
> > but this was an incorrect conversion because an empty string
> > is a valid regular expression.  Removing this check should
> > fix the test failures.
> >
> > Modified:
> >    lldb/trunk/source/Core/RegularExpression.cpp
> >
> > Modified: lldb/trunk/source/Core/RegularExpression.cpp
> > URL:
> http://llvm.org/viewvc/llvm-project/lldb/trunk/source/Core/RegularExpression.cpp?rev=282090&r1=282089&r2=282090&view=diff
> >
> ==============================================================================
> > --- lldb/trunk/source/Core/RegularExpression.cpp (original)
> > +++ lldb/trunk/source/Core/RegularExpression.cpp Wed Sep 21 12:13:51 2016
> > @@ -102,7 +102,7 @@ bool RegularExpression::Compile(llvm::St
> > //---------------------------------------------------------------------
> > bool RegularExpression::Execute(llvm::StringRef str, Match *match) const
> {
> >   int err = 1;
> > -  if (!str.empty() && m_comp_err == 0) {
> > +  if (m_comp_err == 0) {
> >     // Argument to regexec must be null-terminated.
> >     std::string reg_str = str;
> >     if (match) {
> >
> >
> > _______________________________________________
> > lldb-commits mailing list
> > lldb-commits at lists.llvm.org
> > http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/lldb-commits/attachments/20160921/87e9f693/attachment.html>