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

Do not recommend un-approved preview switches #12425

Merged
merged 1 commit into from Jun 2, 2021

Conversation

Geod24
Copy link
Member

@Geod24 Geod24 commented Apr 12, 2021

Neither '-preview=in' nor '-previiew=revaluerefparam' has been approved by the BDFL.
'-preview=in' will require a proper DIP process before being made the default,
and DIP1016 which '-preview=revaluerefparam' implements has been rejected.
Moreover, such a message would only show up in a very narrow corner case:
in the semantic of a default argument which is 'ref' but has a rvalue.

@dlang-bot
Copy link
Contributor

Thanks for your pull request, @Geod24!

Bugzilla references

Your PR doesn't reference any Bugzilla issue.

If your PR contains non-trivial changes, please reference a Bugzilla issue or create a manual changelog.

Testing this PR locally

If you don't have a local development environment setup, you can use Digger to test this PR:

dub run digger -- build "master + dmd#12425"

@Geod24
Copy link
Member Author

Geod24 commented Apr 12, 2021

See conversation here: #12413 (comment)

Copy link
Member

@ibuclaw ibuclaw left a comment

Choose a reason for hiding this comment

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

All tests need to be changed back though.

Copy link
Contributor

@thewilsonator thewilsonator left a comment

Choose a reason for hiding this comment

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

On the contrary, if people are looking for this behaviour and expect it to work (e.g. because they are coming from C++) telling them what switch they should activate to enable it is the most useful information the compiler can provide.

The exact message can be reworded to be more neutral about it but the information should still be provided.

Also preview=rvaluerefparam is not exactly unapproved

@Geod24
Copy link
Member Author

Geod24 commented Apr 12, 2021

On the contrary, if people are looking for this behaviour and expect it to work (e.g. because they are coming from C++) telling them what switch they should activate to enable it is the most useful information the compiler can provide.

See my comment, this only triggers in a very narrow case which isn't very helpful to the user, not consistent.

The exact message can be reworded to be more neutral about it but the information should still be provided.

I disagree.

Also -preview=rvaluerefparam is not exactly unapproved

See discussion here: #12064 (comment)
Note that this is essentially a partial revert of #12064 . If you prefer, I can just submit a full revert of the PR.

@thewilsonator
Copy link
Contributor

thewilsonator commented Apr 12, 2021

this only triggers in a very narrow case which isn't very helpful to the user,

How so? this is much better than nothing.

not consistent.

That is a different problem, one that is fixed by adding more messages like this, not less.

I disagree.

We shall have to agree to disagree, then.

See discussion here: #12064 (comment)

#12064 (comment) , #12064 (comment) , #12064 (comment) (? maybe I'm too tired atm)

I'm not sure what point you're making here.

a partial revert of #12064 . If you prefer, I can just submit a full revert of the PR.

of #12413 ? Not unless -preview=in is a strict superset or equal to -preview=revaluerefparam (see its uses and potential differences) and -preview=in sets -preview=revaluerefparam

@Geod24
Copy link
Member Author

Geod24 commented Apr 12, 2021

How so? this is much better than nothing.

It only triggers for default arguments, while in the majority of cases the users would find it more informative to see this error message on CallExp erroring out.

That is a different problem, one that is fixed by adding more messages like this, not less.

Exactly, fixing the bug is a different problem from informing the user about -preview switch. Hence why I didn't submit a full revert. If you feel strongly about needing to inform the user about those -preview switch, then we should have a PR which goal is to do so, ideally consistently, instead of piggybacking on a bugfix.

#12064 (comment) , #12064 (comment) , #12064 (comment) (? maybe I'm too tired atm)

Yeah, nothing about this is official. I would have no problem recommending it if there was a plan to make it the default. As it stands, the switch will be usable from this release. It obviously needs more discussion and testing before being recommended to unsuspecting user.

of #12413 ?

Yes, that's what I meant. And the reason I mention this revert is, when in disagreement, we tend to revert. The original PR was merged while there was a disagreement on how to proceed with it. II submitted the current PR to fix this. If you don't agree with this (12425) PR, I'll just revert the original PR, because I really don't think we should be recommending those switches. But I hope that's not the route we're going.

Neither '-preview=in' nor '-previiew=revaluerefparam' has been approved by the BDFL.
'-preview=in' will require a proper DIP process before being made the default,
and DIP1016 which '-preview=revaluerefparam' implements has been rejected.
Moreover, such a message would only show up in a very narrow corner case:
in the semantic of a default argument which is 'ref' but has a rvalue.
@Geod24 Geod24 force-pushed the fixup-pr-12413-recommendation-preview branch from 63663cc to 69a1639 Compare April 12, 2021 23:45
@thewilsonator
Copy link
Contributor

while in the majority of cases the users would find it more informative to see this error message on CallExp erroring out.

Yes but this is and not, xor.

If you feel strongly about needing to inform the user about those -preview switch, then we should have a PR which goal is to do so, ideally consistently,

OK, ...

instead of piggybacking on a bugfix.

... (I disagree entirely with this, diagnostics should go with their fixes) ...

because I really don't think we should be recommending those switches.

... but we already do

@RazvanN7
Copy link
Contributor

@Geod24 @thewilsonator how should we proceed here, given #12428 ? Maybe we should continue suggesting -preview=rvaluerefparam , but not -preview=in ?

@RazvanN7
Copy link
Contributor

ping @Geod24 @thewilsonator

@PetarKirov
Copy link
Member

PetarKirov commented Apr 27, 2021

@RazvanN7 I think ultimately @WalterBright and @atilaneves should decide on the policy that we should follow.

@Geod24
Copy link
Member Author

Geod24 commented Apr 28, 2021

Although that was the point of "un-approved". Neither of those switches have seen approval from the BDFL. If it was -preview=dip1000, that would be a different discussion.

@12345swordy
Copy link
Contributor

@razvan I have email Andrei regarding the dip, we are still waiting for an answer from walter for the question that asked by Andrei, regarding the rvalue dip.

@WalterBright WalterBright dismissed thewilsonator’s stale review June 2, 2021 06:50

I understand I wrote the implementation, but that was to try it out. @Geod24 is correct that we shouldn't be recommending it if it is unapproved.

@ibuclaw
Copy link
Member

ibuclaw commented Jun 2, 2021

Merging as per above decision and rationale.

@ibuclaw ibuclaw merged commit fff4f2c into dlang:master Jun 2, 2021
@Geod24 Geod24 deleted the fixup-pr-12413-recommendation-preview branch June 2, 2021 09:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
8 participants