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

[Arthur O'Dwyer via Phabricator via libcxx-commits]

Quuxplusone added inline comments.


================
Comment at: libcxx/include/string:1458
 private:
+    _LIBCPP_HIDE_FROM_ABI static bool __use_sso(size_type __sz) { return __sz < __min_cap; }
+
----------------
IIUC, I think this should be named `__is_less_than_sso_capacity(x)`, or at least `__fits_in_sso(x)`. `__use_sso` reads like an imperative //instruction//, but what this is is a //query//.

Also, the commit message (PR summary) should explain where we're going with this. Are we going to change the body of this function to
```
    return (!__libcpp_is_constant_evaluated()) && (__sz < __min_cap);
```
? If so, then you should preemptively put this function's body on its own line; otherwise you're just making the next diff more complicated. We want the next diff to be as simple as
```
 _LIBCPP_HIDE_FROM_ABI
 static bool __use_sso(size_type __sz) {
-    return __sz < __min_cap;
+    return (!__libcpp_is_constant_evaluated()) && (__sz < __min_cap);
 }
```


================
Comment at: libcxx/include/string:2403-2405
+    return (__builtin_constant_p(__n) && __use_sso(__n))
                ? __assign_short(__s, __n)
                : __assign_external(__s, __n);
----------------
Here and line 2606, I think your change is correct, but it feels really subtle and someone-not-me should look really closely and see if they agree this is right.
IIUC, `__assign_short` doesn't necessarily mean "assign into the SSO buffer"; it means merely "assign into the //currently active// buffer," i.e. "I guarantee `__n` is definitely less than our current `capacity()` (because our capacity is invariably greater than `__min_cap`) and so you don't have to codegen any code for growing the buffer." It's a codegen hack. If we're constant-evaluated, then we're not generating code and so we don't need the hack (and also there is no invariant regarding `__min_cap`). So the hack becomes
```
if (!__libcpp_is_constant_evaluated() && __builtin_constant_p(__n) && __n < __min_cap)
```
which is equivalent to what you wrote. But I'd like the next reviewer to also check and see if they agree.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D116487