IIRC you said the llvm one doesn't support extended regular expressions. If
that's true, please don't do this till that is fixed. I think pretty much
everybody who is using reg expressions expects to be able to use extended
regular expressions.
Jim
> On Sep 21, 2016, at 4:58 PM, Zachary Turner via lldb-commits
> <lldb-commits@lists.llvm.org> wrote:
>
> Incidentally even the STL regex implementation supports specifying the end
> pointer. So I would call the system one deficient in this regard and
> advocate for replacing it sooner rather than later.
>
> On Wed, Sep 21, 2016 at 4:52 PM Zachary Turner <ztur...@google.com> wrote:
> 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 <ztur...@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 <ztur...@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 <ztur...@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 <gclay...@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@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@lists.llvm.org
> > http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
>
> _______________________________________________
> lldb-commits mailing list
> lldb-commits@lists.llvm.org
> http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
_______________________________________________
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
