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

Define all typedef "constants" as statics on the struct that declares their value type #340

Closed
jnm2 opened this issue Jul 27, 2021 · 4 comments · Fixed by #550
Closed

Define all typedef "constants" as statics on the struct that declares their value type #340

jnm2 opened this issue Jul 27, 2021 · 4 comments · Fixed by #550
Assignees
Labels
enhancement New feature or request

Comments

@jnm2
Copy link
Contributor

jnm2 commented Jul 27, 2021

These are nice to have when implementing callbacks that return HRESULT.

image

In particular, as static members on HRESULT, the IDE will natively make it super easy to autocomplete them:

image

image

@jnm2 jnm2 added the enhancement New feature or request label Jul 27, 2021
@AArnott
Copy link
Member

AArnott commented Aug 18, 2021

The metadata defines S_OK, E_FAIL and many others as constants typed as HRESULT.
One idea I've been playing with is making the C# projection lift all these constants as static fields on the structs that they define values for. This would mean that perhaps hundreds of E_ error constants would be defined on HRESULT, and you wouldn't have to ask for any of these constants explicitly in NativeMethods.txt since anything that would generate HRESULT would get all the constants too. Thoughts on that plan?

Alternatively yes we could just specially define a few (e.g. S_OK, S_FALSE, E_FAIL), but then we have to defend why we didn't get certain others that folks will want to see there.

@AArnott AArnott added the discussion needed We need more feedback or responses from ABI experts label Aug 18, 2021
@jnm2
Copy link
Contributor Author

jnm2 commented Aug 18, 2021

That plan sounds great to me!

@AArnott
Copy link
Member

AArnott commented Aug 18, 2021

Ok then here's the overall proposal: move all constants that are typed as a typedef struct to be static members of those structs. So it doesn't just apply to HRESULT.
@jnm2 Are you interested in sending a PR that does that?

@AArnott AArnott changed the title HRESULT.OK and HRESULT.FALSE members Define all typedef "constants" as statics on the struct that declares their value type Sep 18, 2021
@AArnott AArnott removed the discussion needed We need more feedback or responses from ABI experts label Sep 18, 2021
@AArnott AArnott self-assigned this May 16, 2022
AArnott added a commit that referenced this issue May 16, 2022
For example, this moves the `HRESULT PInvoke.S_OK` static field to the `HRESULT` struct.

Closes #340
@AArnott
Copy link
Member

AArnott commented May 16, 2022

Generating all constants into the typedef struct by default has a significant code generation perf and size toll. HRESULT would contain ~9000 constants. I'm dropping that part of the goal. Instead, you still have to request each constant (now with wildcards #544), but it will be generated into the typedef struct, when that struct isn't already found in a referenced assembly.

AArnott added a commit that referenced this issue May 16, 2022
For example, this moves the `HRESULT PInvoke.S_OK` static field to the `HRESULT` struct.

Closes #340
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants