Page MenuHomePhabricator

Implement P0655R1 visit<R>: Explicit Return Type for visit
ClosedPublic

Authored by rarutyun on Nov 24 2020, 10:00 AM.

Diff Detail

Event Timeline

rarutyun created this revision.Nov 24 2020, 10:00 AM
Herald added a project: Restricted Project. · View Herald TranscriptNov 24 2020, 10:00 AM
Herald added 1 blocking reviewer(s): Restricted Project. · View Herald Transcript
rarutyun requested review of this revision.Nov 24 2020, 10:00 AM

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.

rarutyun removed 1 blocking reviewer(s): Restricted Project.Nov 24 2020, 10:06 AM
Herald added 1 blocking reviewer(s): Restricted Project. · View Herald TranscriptNov 24 2020, 10:06 AM

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 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.

mpark added inline comments.Dec 8 2020, 8:19 PM
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());
mpark added a comment.Dec 8 2020, 8:20 PM

@ldionne we probably have discussed the implementation but I don't recall there being a submission.

rarutyun updated this revision to Diff 311031.Dec 10 2020, 2:46 PM
rarutyun removed a reviewer: Restricted Project.
rarutyun marked 3 inline comments as done.Dec 10 2020, 3:15 PM
rarutyun added inline comments.
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.

mpark added inline comments.Dec 18 2020, 3:56 PM
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.

rarutyun updated this revision to Diff 313892.Dec 28 2020, 1:50 PM
rarutyun marked 2 inline comments as done.
rarutyun marked 2 inline comments as done.Dec 28 2020, 1:55 PM
rarutyun added inline comments.
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.

@mpark,

Could you please have a look? I guess all the comments should be addressed

mpark accepted this revision.Jan 8 2021, 1:49 PM
This revision is now accepted and ready to land.Jan 8 2021, 1:49 PM
rarutyun marked an inline comment as done.Jan 11 2021, 1:45 AM

Hi @ldionne,

Michael approved this review. Can we proceed?

ldionne set the repository for this revision to rG LLVM Github Monorepo.Jan 11 2021, 3:09 PM
Herald added a reviewer: Restricted Project. · View Herald TranscriptJan 11 2021, 3:09 PM
This revision now requires review to proceed.Jan 11 2021, 3:09 PM

@rarutyun Can you please rebase onto main so that the CI will trigger? I added the LLVM monorepo tag to the review.

zoecarver accepted this revision.Jan 11 2021, 4:55 PM

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?

rarutyun updated this revision to Diff 316088.Jan 12 2021, 7:32 AM
rarutyun marked 2 inline comments as done.Jan 12 2021, 7:54 AM

@rarutyun Can you please rebase onto main so that the CI will trigger? I added the LLVM monorepo tag to the review.

@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>.

rarutyun marked 2 inline comments as done.Jan 12 2021, 7:54 AM
ldionne set the repository for this revision to rG LLVM Github Monorepo.Jan 18 2021, 11:46 AM
ldionne accepted this revision.Jan 18 2021, 11:48 AM

Do you need help committing this?

This revision is now accepted and ready to land.Jan 18 2021, 11:48 AM

Do you need help committing this?

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.

Do you need help committing this?

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?

rarutyun added a comment.EditedJan 18 2021, 2:54 PM

Do you need help committing this?

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>

thakis added inline comments.
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?)

thakis added inline comments.Jan 25 2021, 12:11 PM
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.