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

[Mark de Wever via Phabricator via libcxx-commits]

Mordante 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; }
+
----------------
Quuxplusone wrote:
> 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);
>  }
> ```
How about directly making the function `constexpr` and implement the final version? That avoids writing a commit message and modifying the code later.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D116487