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

I imagine Greg also thought the llvm classes were just a wrapper around the system reg* functions. I wouldn't have had a problem with that, though it wouldn't have made any noticeable difference to lldb's performance. But I actually had to deal with a bug in the OS X copy of the regex engine

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

Fair enough, greg seemed to be rather disturbed by the copy, so this was really just to appease him, I'm also not too concerned . I do think it's worth sharing code with llvm here, but I'm happy to put it off unless someone insists (I wasn't planning to do it anyway until greg expressed concern)

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

First off, I bet we could decide for giggles to make 5 or 6 random copies of strings before we pass them to regcomp in lldb and none of our users would notice the difference. It's just not something we do as a hot code path. So avoiding a copy or two is not worth even one new bug in regex

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

I suppose, but at the same time if it's good enough for llvm it seems good enough for us. By using it we contribute to its testing, and if we find something it helps a much larger group of people than just us. Anyway it's you guys' call, but this string copy isn't going to go away otherwise and

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

I was being facetious about the enhancements. I am more serious about bugs. I would really rather use a maintained rather than a "I got this source at some point and use it for some things, but don't rigorously test it" version of regcomp & friends. Can we hold off on that change till we

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

I don't believe so. For the same reason we don't want enhanced on Apple and extended everywhere else. Better if all platforms do the same thing. There may be a case to be made for standardizing on std::regex though, it has many different flavors and has been standardized for some time. Maybe

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

So does the build machinery use the one that is supported on that platform if it is available? They are going to get much wider testing, fixes and even "ENHANCE"ments... Jim > On Sep 21, 2016, at 6:07 PM, Zachary Turner wrote: > > Windows :) > > But that was before

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

Windows :) But that was before std::regex. In theory std::regex would work everywhere (although idk how it performs) On Wed, Sep 21, 2016 at 6:04 PM Jim Ingham wrote: > It seems a little odd that llvm has its own forked copy of Henry Spencer's > old regular expression engine?

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

It seems a little odd that llvm has its own forked copy of Henry Spencer's old regular expression engine? Are there platforms we care about that don't have a maintained version of exactly the same code? Jim > On Sep 21, 2016, at 5:42 PM, Zachary Turner wrote: > > I'll

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

I'll try to address the Regex issue this week (by making everything use llvm extended mode regexes). If someone feels up to the challenge, adding support for \d \s etc etc to llvm's regex implementation would make a lot of people very happy. On Wed, Sep 21, 2016 at 5:38 PM Jim Ingham

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

> On Sep 21, 2016, at 5:25 PM, Greg Clayton via lldb-commits > wrote: > > Yep, and we can't have any regex objects in LLDB using those because they > will only work on Apple and we don't want code like: > > #if defined(__APPLE__) > RegularExpression e("\s+"); >

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

On Wed, Sep 21, 2016 at 5:13 PM Greg Clayton wrote: > > > On Sep 21, 2016, at 4:43 PM, Zachary Turner 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

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

Yep, and we can't have any regex objects in LLDB using those because they will only work on Apple and we don't want code like: #if defined(__APPLE__) RegularExpression e("\s+"); #else RegularExpression e("[ \t]+"); #endif I know "\s" is probably extended, so this was a bad example, but

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

That sounds like a plan. BTW, extended is the one that everyone supports, enhanced is the one that only apple supports. On Wed, Sep 21, 2016 at 5:18 PM Greg Clayton wrote: > And we should check for any "extended" mode regex stuff and get rid of it > since as you said they

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

And we should check for any "extended" mode regex stuff and get rid of it since as you said they are not portable. They tend to be shortcuts for classes of objects and we can just fix the regex to be more explicit about it. For now we would keep the lldb_private::RegularExpression class, have

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

> On Sep 21, 2016, at 4:43 PM, Zachary Turner 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. Where? please show this. I

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

Incidentally, it appears the version of regcomp() on apple supports REG_PEND , although that is not a standard. And apple is already the only platform where enhanced regexes are a thing. So

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

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

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

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 wrote: > Looks like llvm's

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

Actually wait, it doesn't. It just explicitly sets the end pointer. On Wed, Sep 21, 2016 at 4:44 PM Zachary Turner wrote: > Worth noting that llvm::Regex has this constructor: > > > Regex::Regex(StringRef regex, unsigned Flags) { > unsigned flags = 0; > preg = new

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

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 &

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

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

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

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