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

Some templates' internal members are not promoted to public #457

Closed
AArnott opened this issue Nov 29, 2021 Discussed in #456 · 3 comments · Fixed by #458
Closed

Some templates' internal members are not promoted to public #457

AArnott opened this issue Nov 29, 2021 Discussed in #456 · 3 comments · Fixed by #458
Labels
bug Something isn't working

Comments

@AArnott
Copy link
Member

AArnott commented Nov 29, 2021

Discussed in #456

Originally posted by AliveDevil November 25, 2021
I don't see any benefit of internalizing ThrowOnFailure: https://github.com/microsoft/CsWin32/blob/main/src/Microsoft.Windows.CsWin32/templates/HRESULT.cs#L43-L62

Same with Succeeded and Failed, consuming these members in any dependent project is impossible, unless InternalsVisibleTo is set.

We should promote internal members from templates to public when the NativeMethods.json contains public: true.

@AArnott AArnott added the bug Something isn't working label Nov 29, 2021
@AArnott AArnott changed the title Any reason the template HRESULT members are internal? Generated internal members from templates should be promoted to public Nov 29, 2021
@AliveDevil
Copy link

You could set the internal template members always to public, as those are never more accessible than the parent scope.
like

internal HRESULT {
public ThrowOnFailure();
}

@AArnott
Copy link
Member Author

AArnott commented Nov 29, 2021

True. That would be the simplest fix. However I dislike defining public members in an internal class because it gives the false impression or at least the possibility that the members are public. In code reviews where I see members added or changed with public modifiers, I have to go double-check the containing type(s) to establish whether they really are public since adding public members carries more cost than adding internal members. I like to save the time for code reviewers by just marking them internal if that's what they really are.

AArnott added a commit that referenced this issue Nov 29, 2021
We were only promoting members in templates whose declaring types explicitly specified a visibility modifier.
Fixes #457
@AArnott AArnott changed the title Generated internal members from templates should be promoted to public Some templates' internal members are not promoted to public Nov 29, 2021
@AArnott
Copy link
Member Author

AArnott commented Nov 29, 2021

It turns out we already had the promotion code, but it had a filter on it that caused it to miss promotion of some of them.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants