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

[Louis Dionne via Phabricator via libcxx-commits]

ldionne accepted this revision.
ldionne added a comment.
This revision is now accepted and ready to land.

This is not a NFC -- the commit shouldn't say `NFC`.



================
Comment at: libcxx/include/string:2403-2405
+    return (__builtin_constant_p(__n) && __use_sso(__n))
                ? __assign_short(__s, __n)
                : __assign_external(__s, __n);
----------------
Quuxplusone wrote:
> 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.
That sounds right to me.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D116487