[libcxx-commits] [PATCH] D116487: [libc++][NFC] Introduce __fits_in_sso()

[Louis Dionne via Phabricator via libcxx-commits]

ldionne requested changes to this revision.
ldionne added a comment.
This revision now requires changes to proceed.

Are there any tests that need to be modified? I would assume that tests that check for reallocation when a string is assigned-to (or similar stuff) will need to be conditionalized on whether the test is running inside a constexpr context too, right?

Finally, and most importantly -- is this implementation strategy really valid? I had thought about this when I originally wrote the constexpr string paper, and IIRC one of the issues was this:

  constexpr std::string s = "short"; // Anything short enough to normally use the SSO. Since this is inside constexpr, though, SSO isn't used.
  
  void use(std::string const* str) {
    // ...
  }
  
  int main() { use(s); }

The problem here is that when `s` is constructed, it is a constexpr context so we don't use the SSO. When we actually *use* `s`, though, it's not a constexpr context so if we check `s`'s size and ask whether it is using SSO, we'll believe that yes, we are using it (because it would normally fit in the small buffer).

Is this not an issue somehow? It's been a while since I've thought about this.



================
Comment at: libcxx/include/string:1459
+    _LIBCPP_CONSTEXPR _LIBCPP_HIDE_FROM_ABI static bool __fits_in_sso(size_type __sz) {
+        return !__libcpp_is_constant_evaluated() && (__sz < __min_cap);
+    }
----------------
Can you add a comment saying something like "We never use the SSO in constexpr contexts because it requires some amount of reinterpret-casting, which is not constexpr-friendly"?

Otherwise, one would rightly wonder why a function named `__fits_in_sso` would return something different based on whether it is called in a constexpr context or not.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D116487/new/

https://reviews.llvm.org/D116487