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

[David Zarzycki via Phabricator via llvm-commits]

davezarzycki added inline comments.


================
Comment at: include/llvm/ADT/StringSwitch.h:49
   /// null before that.
-  const T *Result;
+  Optional<T> Result;
 
----------------
jordan_rose wrote:
> This makes it impossible to return a move-only type by reference (although I suppose you can still do it by pointer).
Doug signed of on this in a separate and simpler review.


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


Repository:
  rL LLVM

https://reviews.llvm.org/D43436