-
Notifications
You must be signed in to change notification settings - Fork 804
P3016R6 Resolve inconsistencies in begin/end for valarray and braced initializer lists #8544
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
P3016R6 Resolve inconsistencies in begin/end for valarray and braced initializer lists #8544
Conversation
…initializer lists
e507db3 to
d21204a
Compare
It is tricky to properly line-break the list of additional headers that contain the range functions, and that list is growing quite large to be displayed as a list in a single sentence. Prefer to render the list as a table instead. Retained the cross-references to the header synopses from the original text, but they look odd in the table format.
fda8120 to
09fe942
Compare
eisenwave
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some discrepancies.
Especially make sure that the noexcept(noexcept(...)) contents are correct.
| #define @\defnlibxname{cpp_lib_apply}@ 202506L // freestanding, also in \libheader{tuple}, \libheader{type_traits} | ||
| #define @\defnlibxname{cpp_lib_apply}@ 202506L | ||
| // freestanding, also in \libheader{tuple}, \libheader{type_traits} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't see this in the paper, so it should presumably done in a different PR because it's totally unrelated.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This entry is merely reformatted. I think such change isn't necessary for CI (otherwise, CI should have been failing before) and should be dropped.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The reformatting avoids splitting <type_- and traits> across two lines, which is an improvement. But it should be a separate commit.
| \end{itemdescr} | ||
|
|
||
| \rSec2[valarray.range]{\tcode{valarray} range access} | ||
| \rSec2[valarray.range]{range access} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| \rSec2[valarray.range]{range access} | |
| \rSec2[valarray.range]{Range access} |
| \indexlibrarymember{begin}{valarray}% | ||
| \indexlibrarymember{valarray}{begin}% |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The old way was correct, the way it's described in our specification style guidelines.
| \indexlibrarymember{end}{valarray}% | ||
| \indexlibrarymember{valarray}{end}% |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same here, the old style was correct.
| constexpr const E* data() const noexcept; | ||
| constexpr size_t size() const noexcept; // number of elements | ||
| constexpr bool empty() const noexcept; | ||
| constexpr const E* begin() const noexcept; // first element | ||
| constexpr const E* end() const noexcept; // one past the last element |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
While the paper doesn't propose any comments in this area, it's inconsistent to have "tutorial comments" on only some of these functions within this block. Maybe a follow-up commit (possibly on a separate PR) could just remove all of those.
I don't think we need code comments like // number of elements telling us that size() returns the number of elements.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree with removing them, but it should definitely be a separate PR not as part of this change.
| template<class C> constexpr auto | ||
| begin(C& c) noexcept(noexcept(c.begin())) -> decltype(c.begin()); // freestanding | ||
| template<class C> constexpr auto | ||
| begin(const C& c) noexcept(noexcept(c.begin())) -> decltype(c.begin()); // freestanding | ||
| template<class C> constexpr auto | ||
| end(C& c) noexcept(noexcept(c.end())) -> decltype(c.end()); // freestanding | ||
| template<class C> constexpr auto | ||
| end(const C& c) noexcept(noexcept(c.end())) -> decltype(c.end()); // freestanding |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To be honest, I liked the style in the paper better,
which is
template<class C> constexpr auto begin(C& c)
noexcept(noexcept(c.begin())) -> decltype(c.begin());There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We usually break after the template-head, and then maybe also break later in the declaration. But I think the <iterator> synopsis is unusual (unique even?) in doing:
template<class C> constexpr auto
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i.e. I think it would be move conventional to do:
template<class C>
constexpr auto begin(C& c) noexcept(...) -> decltype(...); // freestanding
and if that doesn't fit, then something like:
template<class C>
constexpr auto begin(C& c) noexcept(...)
-> decltype(...); // freestanding
| template<class C> constexpr auto crbegin(const C& c) noexcept(noexcept(c.crbegin())) | ||
| -> decltype(std::rbegin(c)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| template<class C> constexpr auto crbegin(const C& c) noexcept(noexcept(c.crbegin())) | |
| -> decltype(std::rbegin(c)); | |
| template<class C> constexpr auto crbegin(const C& c) noexcept(noexcept(std::rbegin(c))) | |
| -> decltype(std::rbegin(c)); |
Same mistake of using the member functions instead of the std:: variant in a few other places.
| \indexlibrary{\idxcode{ssize(C\& c)}}% | ||
| \begin{itemdecl} | ||
| template<class C> constexpr auto ssize(const C& c) | ||
| template<class C> constexpr auto ssize(const C& c) noexcept(noexcept(c.ssize())) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| template<class C> constexpr auto ssize(const C& c) noexcept(noexcept(c.ssize())) | |
| template<class C> constexpr auto ssize(const C& c) noexcept(noexcept(c.size())) |
Fixes #8462