[PATCH] D43436: [Perf] Simplify llvm::StringSwitch and improve incremental rebuild by 54%

[Jordan Rose via Phabricator via llvm-commits]

jordan_rose added inline comments.


================
Comment at: include/llvm/ADT/StringSwitch.h:73
   LLVM_ATTRIBUTE_ALWAYS_INLINE
-  StringSwitch& Case(const char (&S)[N], const T& Value) {
-    assert(N);
-    if (!Result && N-1 == Str.size() &&
-        (N == 1 || std::memcmp(S, Str.data(), N-1) == 0)) {
-      Result = &Value;
+  StringSwitch &Case(StringRef S, T Value) {
+    if (!Result && Str == S) {
----------------
davezarzycki wrote:
> craig.topper wrote:
> > davezarzycki wrote:
> > > davezarzycki wrote:
> > > > jordan_rose wrote:
> > > > > Until StringRef gets a known-size constructor I don't think we should do this, even if it's not kicking in in most cases. It's also serving another purpose right now, which is to make sure the argument strings are compile-time constants. I can see that it would generate tons of duplicate code this way, though.
> > > > > 
> > > > > (StringRef has long been overdo for a constexpr known-size constructor anyway.)
> > > > strlen() is effectively constexpr (I just verified this in a test file). Unless there is something nontrivial I'm missing about StringRef that would prevent strlen() from being compile away, there should be no difference in code output.
> > > Oh, I also tried adding a known size constructor to StringRef and that blew up spectacularly. Too much code likes to create large fixed size char arrays, fill in the first few bytes, and then call StringRef(array) and trust that strlen() finds the NUL byte.
> > There's a subclass of StringRef called StringLiteral that has a fixed size constructor. Can that be used here?
> Neat-o. I spent hours yesterday trying to figure out if a mix of type traits and template magic could detect string literals inside of StringRef, but I didn't think of just subclassing StringRef and I forgot that __builtin_strlen() existed. That should work.
Oh, whoa, I didn't know that existed. Thanks, Craig! (Now to go use it in some places in Swift…)


Repository:
  rL LLVM

https://reviews.llvm.org/D43436