[libcxx-commits] [PATCH] D114395: [libc++] Fix the return value of max_size()

Marshall Clow via Phabricator via libcxx-commits libcxx-commits at lists.llvm.org
Mon Nov 22 16:31:55 PST 2021


mclow.lists added inline comments.


================
Comment at: libcxx/include/string_view:334
     _LIBCPP_CONSTEXPR _LIBCPP_INLINE_VISIBILITY
-    size_type max_size() const _NOEXCEPT { return numeric_limits<size_type>::max(); }
+    size_type max_size() const _NOEXCEPT { return (numeric_limits<size_type>::max() - sizeof(*this)) / sizeof(value_type); }
 
----------------
Quuxplusone wrote:
> (1) This seems unnecessarily fiddly; I don't really see anything wrong with the original.
> (2) If we're going to do this, let's add a line break somewhere.
> (3) Since `sv.size()` cannot physically return anything bigger than `PTRDIFF_MAX`, you probably want to use `ptrdiff_t` instead of `size_type` here... or maybe return `std::min(PTRDIFF_MAX, SIZE_MAX / sizeof(value_type))`? But again, this is needlessly fiddly and I see nothing wrong with just returning `SIZE_MAX` unconditionally. Literally nobody should ever be calling this method.
> 
> Do any of the other methods (e.g. the constructor) need to do something special to preserve the postcondition that `sv.size() <= sv.max_size()`? Before this change, the postcondition was vacuously true; now, it could conceivably be false, and maybe we need to check for that?
My preference is `return (numeric_limits<size_type>::max() / sizeof(value_type)` since this is the max number of elements that a `string_view` can hold.

As for "literally nobody should be calling this method", I agree, with the exception of people writing test suites.


================
Comment at: libcxx/test/std/strings/string.view/string.view.capacity/capacity.pass.cpp:51
+    // has a bug.
+    {
+        typedef typename SV::value_type CharT;
----------------
this is just not true, IMHO.

The sequence of `value_type`s that a `string_view`s pointer points at can (theoretically) be up to `size_t` bytes long.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D114395



More information about the libcxx-commits mailing list