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

Allows custom windows-targets function visibility. #2463

Closed
wants to merge 1 commit into from
Closed

Allows custom windows-targets function visibility. #2463

wants to merge 1 commit into from

Conversation

rodrigocfd
Copy link

Currently the link! macro only produces functions with pub visibility. I'm looking forward to add windows_targets as a dependency to winsafe – as proposed here and clarified here – and being able to specify the function visibility would be handy.

@rodrigocfd
Copy link
Author

rodrigocfd commented Apr 20, 2023 via email

@kennykerr
Copy link
Collaborator

Please open an issue. This change alone will not work as the other crates that depend on windows-targets would need to agree on how they use the link macro.

@kennykerr kennykerr closed this Apr 20, 2023
@rodrigocfd
Copy link
Author

rodrigocfd commented Apr 20, 2023

This change alone will not work as the other crates that depend on windows-targets would need to agree on how they use the link macro.

The vis attribute can be empty, so no changes would be needed on the user's side.

@kennykerr kennykerr reopened this Apr 20, 2023
@kennykerr
Copy link
Collaborator

Fair enough, I'll let the build validate.

@kennykerr
Copy link
Collaborator

The vis attribute can be empty, so no changes would be needed

I think the problem is going to be that existing code expects it to be public.

@kennykerr
Copy link
Collaborator

You should be able to use the link macro within a module, typically called bindings, and then control the visibility from the outer module. That should give you what you need without changes to the link macro.

@rodrigocfd
Copy link
Author

You should be able to use the link macro within a module, typically called bindings, and then control the visibility from the outer module. That should give you what you need without changes to the link macro.

That's exactly how I'm using it today, I confined all declarations whithin a module, so they won't escape to public visibility. I saw the change as a good opportunity to make it more strictly correct.

However we have a problem now. If "no visibility" is informed (empty $vis), Rust sees as "module private" visibility. However, the current link! implementation interprets "no visibility" as pub, because $vis is never informed today (it simply doesn't exist).

So adding $vis to the link! macro would fundamentally change the link! signature, and all users would be forced to update its usage, because everything would be private by default. It would be fairly easy to catch though – the compiler would immediately inform that the funcions are private.

But is such a change acceptable?

@kennykerr
Copy link
Collaborator

I'm not opposed to it in theory, but it would be a breaking change and there just doesn't seem to be enough reason to warrant that right now.

@rodrigocfd
Copy link
Author

I agree. Since the visibility can me mitigated by wrapping the declarations inside a module, a breaking change doesn't seem reasonable by now.

If needed in the future, however, the seed has been planted.

@rodrigocfd rodrigocfd closed this Apr 20, 2023
@kennykerr
Copy link
Collaborator

Yep, I wish I thought of it sooner. 😜 Thanks for the suggestion!

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.

2 participants