[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:49
   /// null before that.
-  const T *Result;
+  Optional<T> Result;
 
----------------
This makes it impossible to return a move-only type by reference (although I suppose you can still do it by pointer).


================
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) {
----------------
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.)


Repository:
  rL LLVM

https://reviews.llvm.org/D43436