On Tue, 10 Jun 2025 at 15:17, Giuseppe D'Angelo
<giuseppe.dang...@kdab.com> wrote:
>
> 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.
We haven't replaced all the existing uses of __addressof (yet) but we
can stop adding more uses :-)
>
>
> >
> >> + );
> >> + }
> >> +
> >> + 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?
No, and it could easily be changed to (int)has_value() if needed.
> 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.
Ack
> >
> > + test(std::string("test"));
>
> 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?
Would string_view work instead? If not, please change it to vector.
