[libcxx-commits] [PATCH] D75554: Partially inline basic_string::assign(__s [, __n]) methods.

[Louis Dionne via Phabricator via libcxx-commits]

ldionne requested changes to this revision.
ldionne added inline comments.
This revision now requires changes to proceed.


================
Comment at: libcxx/include/string:1575
+    // `__assign_inline` contains the implementation called by the externally
+    // instantiated __assign_no_alias() and__assign_maybe_alias() methods.
+    template <bool __is_short, bool __maybe_aliased>
----------------
Nitpick: `and__assign_maybe_alias` is missing a space


================
Comment at: libcxx/include/string:1581
+    // into the current instance where the caller has verified both
+    // that the it fits capacity, as well as that there is no aliasing.
+    inline void __assign_in_place(const value_type* __s, size_type __n) {
----------------
nitpick typo: `that the it fits`


================
Comment at: libcxx/include/string:2271
+    const value_type* __s) {
+  size_type __n = traits_type::length(__s);
+  __is_long() ? __assign_inline<false, true>(__s, __n)
----------------
We could call `__assign_maybe_alias(__s, traits_type::length(__s))` instead to avoid code duplication?


================
Comment at: libcxx/include/string:2283
+    if (__n < __min_cap) {
+      __assign_in_place(__s, __n);
+    } else {
----------------
Just to confirm my understanding: at this point we don't know whether `*this` is short or long, however we do know `[__s, __s + __n)` is going to fit in whatever string we have, cause `__n` would fit in the short string.

However, why do we know there's no aliasing (which `__assign_in_place` assumes)?


================
Comment at: libcxx/include/string:2479
+  _LIBCPP_ASSERT(__s != nullptr, "string::assign received nullptr");
+  if (_LIBCPP_BUILTIN_CONSTANT_P(__s[0])) {
+    const size_type __n = traits_type::length(__s);
----------------
Why don't we instead do:

```
if (_LIBCPP_BUILTIN_CONSTANT_P(__s[0])) {
    assign(__s, traits_type::length(__s));
} else {
    __assign_maybe_alias(__s);
}
```

I think both are equivalent, but we duplicate less code.



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D75554