[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