Skip to content
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

Make cpp2::args usable as a std::ranges::bidirectional_range #1281

Open
wants to merge 6 commits into
base: main
Choose a base branch
from

Conversation

jhugard
Copy link

@jhugard jhugard commented Sep 16, 2024

Satisfy the std::ranges::bidirectional_range constraint, so that cpp2::args can be used with std::ranges adapters, etc.

See #1280.

@hsutter
Copy link
Owner

hsutter commented Sep 23, 2024

Thanks for your pull request! It looks like this may be your first contribution to cppfront. I've emailed you the Contributor License Agreement (CLA), and once it's signed I can look at your pull request. Thanks again for your contribution.

@jhugard jhugard force-pushed the range-compatible-main-args branch from be19dc9 to 759493c Compare September 27, 2024 08:43
include/cpp2util.h Outdated Show resolved Hide resolved
include/cpp2util.h Outdated Show resolved Hide resolved
constexpr iterator(const iterator& o) = default;
constexpr iterator(iterator&& o) = default;
constexpr iterator& operator=(const iterator& o) = default;
constexpr iterator& operator=(iterator&& o) = default;
Copy link
Contributor

@gregmarr gregmarr Sep 27, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Did it not work without explicitly defaulting the copy/move functions?

Copy link
Author

@jhugard jhugard Sep 28, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, it does not. Apologies for patronizing, but because a constructor (with parameters) was specified, the default ctors and assignment are suppressed.

Rule-of-zero, rule-of-five says if you define one, you should define them all: see core guidelines C.ctor C.21.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Defining any constructor suppresses the default constructor, which is why you need a version with =default for that.

However, the default constructor is not part of the rule of 0/5 from C.21. Those are copy/move construct/assign, and destructor. They should all still be generated unless you define one of those 5.

https://en.cppreference.com/w/cpp/language/copy_constructor

If no user-defined copy constructors are provided for a class type, the compiler will always declare a copy constructor as a non-explicit inline public member of its class.

Here's a table form of that information: https://mariusbancila.ro/blog/2018/07/26/cpp-special-member-function-rules/

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

TIL, thank you! I stand 100% corrected and have apparently misunderstood the rule of five for some time: I had been including the default constructor in that count, rather than the dtor. Appreciate your link to the table form. Very clear.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

TIL, thank you!

You're welcome.

I had been including the default constructor in that count, rather than the dtor.

That's an easy mistake to make. I'm pretty sure I've included it in the past. :)

@gregmarr
Copy link
Contributor

gregmarr commented Oct 4, 2024

@jhugard Did you return the CLA?

@jhugard
Copy link
Author

jhugard commented Oct 8, 2024

@jhugard Did you return the CLA?

I am still waiting for approval from our legal department, which could take a while.

@gregmarr
Copy link
Contributor

gregmarr commented Oct 9, 2024

@jhugard Did you return the CLA?

I am still waiting for approval from our legal department, which could take a while.

Ugh, I feel your pain. :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants