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

David Zarzycki via Phabricator via llvm-commits llvm-commits at lists.llvm.org
Mon Feb 19 09:38:51 PST 2018


davezarzycki 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:
> 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.


Repository:
  rL LLVM

https://reviews.llvm.org/D43436





More information about the llvm-commits mailing list