Details
- Reviewers
ldionne zoecarver mpark - Group Reviewers
Restricted Project - Commits
- rG51faba35fd81: [libc++] Implement P0655R1 visit<R>: Explicit Return Type for visit
Diff Detail
Event Timeline
Didn't find corresponding feature macro. Let me know if we need to do something with that.
| libcxx/test/std/utilities/variant/variant.visit/visit_return_type.pass.cpp | ||
|---|---|---|
| 247 | I decided to check the constexpr functionality for void return type with the operator comma. Please let me know if it makes sense from your perspecitve to test constexpr-ness of the function that returns void. To me it doesn't have much sense but I have done in case it's necessary. | |
Michael, can you have a look since you wrote that paper? I also faintly remember that we already tried implementing this in libc++, didn't we?
I looked at the status and tried to find the open reviews with no luck before starting the implementation.
| libcxx/include/variant | ||
|---|---|---|
| 1642 | When we initially implemented this, there were shaky C++17 support for some compilers. It might be fine now... I guess we'll see if any of the build configs break. | |
| 1667 | nit: formatting | |
| libcxx/test/std/utilities/variant/variant.visit/visit.pass.cpp | ||
| 357 | This is now testing visit<void>(...) rather than visit(...), right? Don't we want to continue to test visit(...); here? | |
| libcxx/test/std/utilities/variant/variant.visit/visit_return_type.pass.cpp | ||
| 247 | I agree with you that it doesn't really make much sense. But these tests didn't exist, so I'm not really following why you felt the need to add them if it doesn't make sense to you. Is the intention that constexpr returning void should be tested? If so, why weren't they added to visit.pass.cpp as well...? Anyway, I think to test constexpr void, we should be testing that we can perform mutating operations within a constexpr function, using a constexpr void function. Something like: auto test = []() constexpr {
std::variant<int> v = 101;
std::visit<void>([](int& x) constexpr { x = 202; }, v);
return get<int>(v) == 202;
};
static_assert(test()); | |
@ldionne we probably have discussed the implementation but I don't recall there being a submission.
| libcxx/include/variant | ||
|---|---|---|
| 1642 | Yes, agree. I may return it back if we'll see some failures. I propose to leave it as is for now | |
| libcxx/test/std/utilities/variant/variant.visit/visit.pass.cpp | ||
| 357 | Good catch. Thanks. Fixed. | |
| libcxx/test/std/utilities/variant/variant.visit/visit_return_type.pass.cpp | ||
| 247 | Let me explain my thoughts. I doubt if the similar test should be added to just visit because visit always has the same return type as the visitor. For the visit<R> user may explicitly speсify the return type including void. That means we have some kind of branch where the return value of visitor can be either converted to R or can be ignored. Despite that, the function has to be constexpr. I added such tests to cover both those branches even if void one doesn't have much sense in real code. I also considered to create some wrapper function (similar to your code snippet) where I would use constexpr visit<void> but I decided to use the same tests visit has with operator comma. I don't see a big difference with what you suggest vs what I did because both tests check if the function can be used in constexpr context. The only difference is your test is more close to the real code use-cases. Overall: I may add the test you suggest but we may still have the tests I wrote (with or without applying the suggestion) if you agree with my argumentation. If you think that tests I wrote create more confusion than value they may be removed. | |
| libcxx/test/std/utilities/variant/variant.visit/visit_return_type.pass.cpp | ||
|---|---|---|
| 247 | I think keeping these tests are fine, the reason for my suggestion was due to my experience in seeing at least one situation where Clang allowed (non-constexpr, constexpr) by completely ignoring the non-constexpr portion of the expression. The way I presented above just ensures not only that a non-constexpr can be mentioned within a constexpr context, but also that its side-effects are observed. | |
| libcxx/test/std/utilities/variant/variant.visit/visit_return_type.pass.cpp | ||
|---|---|---|
| 247 | Ok. I returned from vacation:) and ready to finish this review. I have added test you suggested with the slight modifications. I have also rebase the change with the latest main branch. Could you please have a look? I guess all the comments should be addressed | |
@rarutyun Can you please rebase onto main so that the CI will trigger? I added the LLVM monorepo tag to the review.
Didn't read it too carefully, but this looks good to me.
| libcxx/include/variant | ||
|---|---|---|
| 631 | Nit: these can simply be invoke now that D93815 landed. | |
| libcxx/test/std/utilities/variant/variant.visit/visit_return_type.pass.cpp | ||
| 336 | Is there a chance this will throw anything other than bad variant access? | |
| libcxx/test/support/variant_test_helpers.h | ||
| 15 | Is this header needed? | |
@ldionne, sure, done.
| libcxx/include/variant | ||
|---|---|---|
| 631 | Ok, thanks. I would prefer to leave it as is because we have lots of __invoke_constexpr usage in the code. If we want to refactor that file I believe it should be the separate patch. | |
| libcxx/test/std/utilities/variant/variant.visit/visit_return_type.pass.cpp | ||
| 336 | I don't think so for now and I believe this test makes sure that only std::bad_variant_access can be thrown. I cannot say for C++ future, of course:) But that's the state of the art we have. I just wanted to make sure that std::visit with explicit return type has non less test set than regular std::visit does where applicable. | |
| libcxx/test/support/variant_test_helpers.h | ||
| 15 | Thanks for noticing. Fixed. This header was needed unless I moved specific tests for visit and visit<R>. | |
I think so.
I would prefer to go through this path myself, to make commits directly (if it's possible and if I have enough permissions to do that). It's just may save the time of other people and your time in particular:) Currently, I just don't know what to do. So if you (or somebody else) can teach me it would be great.
But I don't mind if you merge it yourself. Please choose the way that works better for you.
The steps to gain commit access are explained here: https://llvm.org/docs/DeveloperPolicy.html#obtaining-commit-access. It's generally for people with a history of submitting patches already, so I'll let you judge whether you think it's right for you or not.
In the meantime, I'm happy to apply your patch. Can you please provide Author Name <email@domain> so I can properly attribute the commit?
Ok. Currently I am planning to continue my contribution, so let me probably to request that access later after a couple of more commits.
Information for the commit: Ruslan Arutyunyan <ruslan.arutyunyan@intel.com>
| libcxx/include/variant | ||
|---|---|---|
| 657 | Should this endif be a line further up? This comments out the end of the class. My -std=c++17 bot is red because of this: http://45.33.8.238/linux/37893/step_4.txt (Is c++20 required to build libc++ now?) | |
| libcxx/include/variant | ||
|---|---|---|
| 657 | I went ahead and pushed a fix for this at a206d991f96ba5c864aa6527f87b72ab1fdf3f4c | |
Thanks a lot Nico. The back story is that CI didn't run for that revision due to a misconfigured repository on the Phab revision. This is a great reminder that we should never skip pre-commit CI for non-trivial changes. This is my fault, I should have requested that the author updates the patch with a proper repository set so CI would trigger.
Nit: these can simply be invoke now that D93815 landed.