Details
- Reviewers
ldionne zoecarver - Group Reviewers
Restricted Project - Commits
- rGa11f8b1ad66d: [libc++] [P0935] [C++20] Eradicating unnecessarily explicit default…
Diff Detail
- Repository
- rG LLVM Github Monorepo
Unit Tests
Event Timeline
libcxx/include/random | ||
---|---|---|
101–104 | Please disregard modifications to stuff other than linear_congruential_engine. |
This is the same as D60422, which hasn't been updated in a while. I left some comments regarding how I suggest we create shared code to check for implicit default constructibility in that review. But I like the make_implicit<E>() trick.
@zoecarver I think you two should coordinate.
@ldionne, 've discussed it with @zoecarver, and we decided that I'll take it over. It might take some time as there are many things to change (and test), but I'll appreciate your feedback.
My doubts:
- is using make_implicit really necessary or just test_convertible is enough? Using make_implicit tests as well that the default parameter is the correct one. It's not always possible (as in random_device for instance).
- name and location of make_implicit?
- I added as well test for ctors being explicit, and that was not tested before.
One more question, lots of stuff in <random> tests are also run in c++03. Is it expected? Should I add guards to my additions (#if TEST_STD_VER >= 11)?
- Use typedef for C++03 compatibility.
- [rand.dist.bern]
- [rand.dist.pois]
- [rand.dist.norm]
- [stringbuf]
- [istringstream]
- [stringbuf]
- [ostringstream]
- [stringstream]
- [re.results]
- [depr.strstreambuf]
- [depr.conversions.string]
- [depr.conversions.buffer]
- [queue] [priority.queue]
- [stack]
I think we should test the value of the default argument when we can, as you did.
- name and location of make_implicit?
What you have looks OK to me.
- I added as well test for ctors being explicit, and that was not tested before.
Thank you!
We support <random> in C++03. It's one of those great things :-).
libcxx/test/support/make_implicit.h | ||
---|---|---|
26 | C++11 is required only because you're using a brace initializer, right? Clang supports variadics and rvalue references even in C++03 mode, so I'm just checking. |
libcxx/test/support/make_implicit.h | ||
---|---|---|
26 | Brace initializer, rvalue references and variadics, also std::forward but that could be just a static_cast. I can make it work for C++03 then. |
- Update status page.
- Generalize tests. Clean up.
- Remove requirement for > C++11 in make_implicit.h.
I removed the unnecessary check for > C++11 in make_implicit, but that doesn't change a lot as it's used anyway only in C++20.
Anyway, I've just seen that P0935 has been adopted as a DR applied retroactively to previous standards down to C++11. My only source tho is https://en.cppreference.com/w/cpp/numeric/random/uniform_real_distribution/uniform_real_distribution.
Should I apply it to pre-C++20 then?
It was adopted as a paper, and it does *not* appear to have been voted as a DR. I found on the Rapperswill (2018) wiki motion page:
Motion 25
Move to apply to the C++ working paper the proposed wording in P0935R0 (Eradicating unnecessarily explicit default constructors from the standard library).
It doesn't appear to be applied as a DR, however the author himself was the one to change cppreference (looking at the history). @tcanens Do you have more context?
LWG doesn't do official DRs like ever, so cppreference policy for the library is that we look at what implementations do (or make an educated guess/prediction about what they will do) to call things "DRs". We are trying to document things for programmers, not standard archaeologists or language lawyers.
Both libstdc++ and MSVC implement P0935 unconditionally, so I suspect that I guessed correctly for this case.
I'm not sure I follow. Aren't all LWG issues considered DRs and "back applied" to earlier standards, hence what we do in the libraries wrt LWG issues?
so cppreference policy for the library is that we look at what implementations do (or make an educated guess/prediction about what they will do) to call things "DRs". We are trying to document things for programmers, not standard archaeologists or language lawyers.
I think it's misleading to call it a DR if it isn't one. I believe it would be more correct (and equally useful) to say that most implementations support it back in C++11, without using the word DR. It's a bit pedantic, but I think nothing is lost by being more precise.
Both libstdc++ and MSVC implement P0935 unconditionally, so I suspect that I guessed correctly for this case.
@curdeius In that case, I believe we should support it unconditionally as well. At worst, it's a very very small extension in older standards. That'll also simplify the implementation and the tests a bit.
LWG doesn't move issue resolutions as formal DRs the way CWG does. And not all issues are DRs - see, e.g., LWG2911 aka is_aggregate. I think the "file an issue if you want a paper back applied" process (as in LWG3494) is a fairly new invention.
so cppreference policy for the library is that we look at what implementations do (or make an educated guess/prediction about what they will do) to call things "DRs". We are trying to document things for programmers, not standard archaeologists or language lawyers.
I think it's misleading to call it a DR if it isn't one. I believe it would be more correct (and equally useful) to say that most implementations support it back in C++11, without using the word DR. It's a bit pedantic, but I think nothing is lost by being more precise.
I think that adds unnecessary confusion and complexity, not to mention that ISO rules say that DRs only apply to the immediately previous standard so we still have similar problems with stuff that goes back farther than that. But I don't think this is the right place to discuss cppreference policy.
Indeed, I was just giving my opinion :-). cppreference is a great tool nonetheless.
@curdeius Let's implement this back to C++11 if that's OK by you. That way, we'll remove implementation divergence and simplify our life, and we'll also make cppreference tell the truth. 😁
- Apply to pre-C++20. That requires tho that the compiler supports delegated constructors in C++03 (it seems that clang does).
I didn't look again in detail, but I remember I was OK with the patch except for the question of whether to implement it as a DR. This has been settled now, so this LGTM. Thanks!
Please disregard modifications to stuff other than linear_congruential_engine.