Skip to content

C.60 and C.63: Should we allow operator= to return void? #1988

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

Open
hsutter opened this issue Oct 25, 2022 · 8 comments
Open

C.60 and C.63: Should we allow operator= to return void? #1988

hsutter opened this issue Oct 25, 2022 · 8 comments

Comments

@hsutter
Copy link
Contributor

hsutter commented Oct 25, 2022

We have;

The only reason to require "return by non-const&" is to enable chaining (e.g., a=b=c;).

In recent conversations, I've heard sympathy for not requiring chaining and allowing a void return type from operator=. If so, we should consider changing the Guidelines to allow a void return from operator= so that code that does that won't get flagged by Guideline checkers.

Proposed solution: Changed the end of each the C.60 and C.63 titles to be "... and return by non-const& or void" and update the bodies similarly.

@JohelEGP
Copy link
Contributor

Why not use the suppression attribute specified by https://isocpp.github.io/CppCoreGuidelines/CppCoreGuidelines#inforce-enforcement?

@beinhaerter
Copy link
Contributor

I know you find void return type useful when using = delete. Are there also other cases where one would use void return type? When would I want to disable chaining? Are there cases where chaining could hurt?

Side note: I am not a friend of chaining, I am not using it. But I don't yet see why I should prohibit it to limit clients of my classes.

hsutter referenced this issue in microsoft/GSL Oct 26, 2022
Somewhere along the way, GSL's implementation of final_act and finally seems to have become way overthought. This PR is to re-simplify these facilities back to what C++ Core Guidelines C.30 said which is simple and clear and works. It just copies the invocable thing, and doesn't bother trying to optimize the copy. This should be fine, because we're typically passing something that's cheap to copy, often a stateless lambda.

The problem in #846 appears to be because finally looks like was originally written as a const&/&& overload (its state at the time that issue was opened)... to eliminate a copy when you invoke it with a temporary. If so, then the && was probably never intended to be a forwarder, but an rvalue reference that tripped over the horrid C++ syntax collision where a && parameter magically instead means a forwarding reference because the type happens to be a template parameter type here. So I suspect the original author was just trying to write an rvalue overload, and the forwarder that's there now was never intended at all.
@hsutter
Copy link
Contributor Author

hsutter commented Oct 26, 2022

Yes, yes, and possibly yes, respectively.

This question happened to come up in a recent conversation among the editors, and I heard sympathy for the position that chaining might be generally undesirable or at least not be carrying its weight. It's not often used, and if we knew what we know today back when it was designed (aka "if we had a time machine") a non-void return and chaining might not be in the language today. That's one reason I just wrote void in the GSL commit here. And then I opened this Issue when you correctly pointed out that the Guidelines actually require a non-void return in C.60 and C.63 today (thanks again for pointing that out).

To be clear, this is not a rant against chaining by any means. We're not proposing taking it out of the language or banning it or anything. This Issue is narrowly about whether it makes sense for the Guidelines to minimally just stop requiring chaining, that's all, because I had heard sympathy for considering it a reasonable design choice.

@JohelEGP
Copy link
Contributor

Proposed solution: Changed the end of each the C.60 and C.63 titles to be "... and return by non-const& or void" and update the bodies similarly.

Why not limit the scope to an Exception item? Raising it to the title and main body seems to me like a step backwards from the standard object concepts, which require chaining and the guidelines are in favor of.

@BenjamenMeyer
Copy link

Not a friend of chaining - I rarely if ever use as I find readability better when minimizing changing, however:

In recent conversations, I've heard sympathy for not requiring chaining and allowing a void return type from operator=. If so, we should consider changing the Guidelines to allow a void return from operator= so that code that does that won't get flagged by Guideline checkers.

This would seem to just add confusion to the language since you could no long rely on the operators to return the correct type when some operators return void - a. non-return value. Operators are confusing enough to setup as it is.

Put another way:

a = b

should always be something to reliably work; when b is void or might return void that's just going to create confusion.

You don't necessarily need chaining (a = b = c) for this to be a problem.

@cubbimew
Copy link
Member

Chaining might be red herring as it's just a special case of lvalue use. Though I agree I can't recall intentionally using assignment expressions that way outside of old C's file-reading loops while ((ch=getchar()) != EOF)

@neithernut
Copy link

neithernut commented Oct 27, 2022

The following might be reasonably common:

auto foo() -> std::unique_ptr<...>;

if (auto thing = foo()) {
   // ...
}

Though it may not be directly affected due to the declaration inside the condition. I'm too lazy to look it up right now. However, presumably many will expect that you can move the declaration out of the condition (any may need to due to being stuck with an old standard):

auto foo() -> std::unique_ptr<...>;

std::unique_ptr<...> thing;
if (thing = foo()) {
   // ...
}

Edit: yes, I'm aware that most would choose the saner option of also moving the initialization out of the condition if they cannot, for whatever reason, declare thing in the condition.

@JohelEGP
Copy link
Contributor

The linked GSL commit is about using void when =deleteing operator=, which is very reasonable. Beyond that, a weird type that wants to prohibit chaining, is not contextually convertible to bool, and wants to be assignable but not a model of assignable_from<T> for all T, seems very niche.

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

No branches or pull requests

6 participants