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

Louis Dionne via Phabricator via libcxx-commits libcxx-commits at lists.llvm.org
Wed Mar 4 06:44:32 PST 2020

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:

    assign(__s, traits_type::length(__s));
} else {

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

  rG LLVM Github Monorepo



More information about the libcxx-commits mailing list