[PATCH] Introduce an optimisation for special case lists with large numbers of literal entries.
Peter Collingbourne
peter at pcc.me.uk
Fri Aug 2 19:00:06 PDT 2013
================
Comment at: include/llvm/Support/Regex.h:82
@@ +81,3 @@
+ /// matches itself and only itself.
+ static bool isLiteral(StringRef Str);
+
----------------
Alexey Samsonov wrote:
> Okay. I think this can get approval from Regex author and be submitted separately (likely, with a unittest).
I don't think Regex has an author/owner as such. It's just a wrapper around a BSD regex implementation which has been touched by a number of people. Eli was the last person to touch it significantly.
I've added a handful of unit tests and split it out to D1278.
================
Comment at: lib/Transforms/Utils/SpecialCaseList.cpp:38
@@ +37,3 @@
+ StringSet<> Strings;
+ Regex *RegEx;
+
----------------
Alexey Samsonov wrote:
> Peter Collingbourne wrote:
> > Alexey Samsonov wrote:
> > > Consider:
> > > a) using OwningPtr<Regex> instead of RegEx*. In this case you wouldn't need SpecialCaseList destructor and Entry ctor.
> > > b) making this a class with private data. You need only: addString(), setRegex() and match() methods.
> > >
> > > I would also appreciate a comment about the intent behind this.
> > > a) using OwningPtr<Regex> instead of RegEx*. In this case you wouldn't need SpecialCaseList destructor and Entry ctor.
> >
> > This won't work because StringMap requires its element type to be copyable and assignable.
> >
> > > b) making this a class with private data. You need only: addString(), setRegex() and match() methods.
> >
> > I'd prefer not to do this, as Entry is only an implementation detail of SpecialCaseList.
> >
> > > I would also appreciate a comment about the intent behind this.
> >
> > Done.
> Why not use
> ~Entry() { delete RegEx; }
> then, instead of iterating over two-level StringMap then?
If StringMap does the equivalent of:
Entry a;
a.RegEx = new Regex;
Entry b = a;
and a and b go out of scope, the Regex will be double freed.
http://llvm-reviews.chandlerc.com/D1150
More information about the llvm-commits
mailing list