Re: [PATCH] libstdc++: add range support to std::optional (P3168)

[Giuseppe D'Angelo]

Hello,

Thank you for the review.

+#ifdef __cpp_lib_optional_range_support // >= C++26
+      // Iterator support.
+      constexpr iterator begin() noexcept
+      {
+       return iterator(
+         this->_M_is_engaged() ? std::__addressof(this->_M_get()) : nullptr

This can use std::addressof. I removed the indirection from addressof
to __addressof so there's no reason to prefer __addressof now, except
in code that needs to compile as C++98.

Done. __addressof was used just a couple of lines below so that why I repeated it here.

+       );
+      }
+
+      constexpr const_iterator begin() const noexcept
+      {
+       return const_iterator(
+         this->_M_is_engaged() ? std::__addressof(this->_M_get()) : nullptr
+       );
+      }
+
+      constexpr iterator end() noexcept
+      {
+       return begin() + has_value();

I know this is precisely how end() is defined in the working draft,
but I'm wondering whether we want to make the implementation of
begin() and end() more similar. So either use has_value() in begin()
instead of this->_M_is_engaged(), or define end() as something like:

      return iterator(this->_M_engaged()
          ? std::addressof(this->_M_get()) + 1, nullptr)

No need to change it in your patch though, I don't have a preference
either way, and it could be changed later if somebody has compelling
rationale why one form is better than another.

Yeah, that's exactly I wrote it in the first iteration, before realizing that end() is specified as code, so I changed it. I'm not sure which is best. Is there any such thing like warnings for using booleans with pointer arithmetic? But as you say, I'll leave it alone for now, the two formulations are ABI compatible so it can be changed later if necessary.

+  test(std::string("test"));

Thank you,

--
Giuseppe D'Angelo

Whops, I again added std::string to a constexpr test, so this will fail on the old ABI. Should I change it to e.g. std::vector, or should I keep it and add a `{ dg-require-effective-target cxx11_abi }` directive?

smime.p7s
Description: S/MIME Cryptographic Signature