[libcxx-commits] [PATCH] D153201: [libc++] Adds string_view constructor overload

[Mark de Wever via Phabricator via libcxx-commits]

Mordante planned changes to this revision.
Mordante added a comment.

Thanks for the review!



================
Comment at: libcxx/include/bitset:835
 
+#if _LIBCPP_STD_VER >= 26
+template <size_t _Size>
----------------
philnik wrote:
> Given that we want to move away from defining member functions out-of-line, maybe we want to jut define the new functions inline directly?
Do we? I must have missed that message. When/where was this discussed? (I'm not objecting against it.)


================
Comment at: libcxx/include/bitset:844-858
+    if (__pos > __str.size())
+        __throw_out_of_range("bitset string pos out of range");
+
+    size_t __rlen = std::min(__n, __str.size() - __pos);
+    for (size_t __i = __pos; __i < __pos + __rlen; ++__i)
+        if (!_Traits::eq(__str[__i], __zero) && !_Traits::eq(__str[__i], __one))
+            __throw_invalid_argument("bitset string ctor has invalid argument");
----------------
philnik wrote:
> I think a helper to deduplicate this code between the overloads above and below wold be a good idea. There is also some pretty obvious optimization opportunity missed by clang for this code, so maybe we can improve on it later.
I really dislike that idea in this patch. I'll create a different patch to do the refactoring first. 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D153201