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

allow 'in ref' for legacy compatibility #15443

Merged
merged 1 commit into from
Jul 22, 2023
Merged

Conversation

WalterBright
Copy link
Member

I don't see a problem with allowing this, and disallowing it breaks existing code:

https://www.digitalmars.com/d/archives/digitalmars/D/Windows_experience_is_atrocious_369807.html#N369818

@Geod24

@dlang-bot
Copy link
Contributor

Thanks for your pull request, @WalterBright!

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#15443"

@WalterBright WalterBright merged commit 9fca8be into dlang:master Jul 22, 2023
@WalterBright WalterBright deleted the inref branch July 22, 2023 15:07
@Geod24
Copy link
Member

Geod24 commented Jul 25, 2023

Then we can never make -preview=in the default. That's the problem.

@John-Colvin
Copy link
Contributor

Then we can never make -preview=in the default. That's the problem.

Editions solves this. Just have to work out what editions are. Oh and implement 😛

@dkorpel
Copy link
Contributor

dkorpel commented Jul 25, 2023

Then we can never make -preview=in the default. That's the problem.

Does -preview=in want to give in ref a new meaning? Otherwise I don't see what the problem is with allowing in ref for legacy.

@anon17
Copy link

anon17 commented Jul 26, 2023

I just realised I have fixed endianness methods, that load integers from byte arrays, so it takes in ref byte[4] data and checks it has right alignment assert((cast(int)data.ptr&3)==0), as it's important for alignment-sensitive platforms, so data should be properly aligned on all platforms, but -preview=in passes in byte[4] data by value, so alignment check doesn't work after removing ref.

@Geod24
Copy link
Member

Geod24 commented Jul 27, 2023

@anon17 : If this matters, by definition, you want to enforce ref. So scope ref const(byte[4]) is what you should use, not -preview=in.
@dkorpel : -preview=in wants to disallow in ref.

dkorpel pushed a commit to dkorpel/dmd that referenced this pull request Jul 27, 2023
@dkorpel
Copy link
Contributor

dkorpel commented Jul 27, 2023

-preview=in wants to disallow in ref

Then there's no need to deprecate in ref quickly, -preview=in could simply tolerate that for a while.

@WalterBright
Copy link
Member Author

Note that the current behavior of -preview=in is passing by ref is implementation dependent.

@schveiguy
Copy link
Member

Then we can never make -preview=in the default. That's the problem.

I don't see how this is true. If in ref just means in, then it's fine. If it gets translated to const scope ref, then it's fine too.

with -preview=in, the choice of ref or not is made by the compiler, but semantically it's the same (you are receiving const data either by value or ref). Why should it matter whether you have redundant attributes? Can't we let it work for the sake of keeping backwards compatibility?

@mw66
Copy link

mw66 commented Jul 31, 2023

I don't see a problem with allowing this, and disallowing it breaks existing code:

https://www.digitalmars.com/d/archives/digitalmars/D/Windows_experience_is_atrocious_369807.html#N369818

@WalterBright off topic: just noticed this forum web interface does not mask off people's email address.

@anon17
Copy link

anon17 commented Aug 15, 2023

Ah, IIRC in ref was deprecated because there was a problem with mangling and Geod24 didn't like that two methods with in and in ref can have the same ABI but different API.

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

Successfully merging this pull request may close these issues.

8 participants