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

CreateConverter flags out parameter for future expansion #2

Closed
wants to merge 1 commit into from

Conversation

sredna
Copy link

@sredna sredna commented Feb 11, 2022

This converts the DataInspector SupportsStrToBytes output parameter to a flags parameter for future expansion.

This slightly breaks ABI if somebody had code that does *SupportsStrToBytes = 2 instead of true/false but this is a non-issue since the current plug-in API is already ABI incompatible with the current HxD release (2.5.0.0).

Copy link
Owner

@maelh maelh left a comment

Choose a reason for hiding this comment

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

BOOL is used because bool's size in bytes is implementation defined, whereas BOOL is guaranteed to be the same size as int. Since Windows follows the LLP64 model, int is 32 bit under x86-32 and also under x86-64.

https://stackoverflow.com/questions/4897844/is-sizeofbool-defined-in-the-c-language-standard
https://docs.microsoft.com/en-us/windows/win32/winprog/windows-data-types
https://docs.microsoft.com/en-us/windows/win32/winprog64/abstract-data-models

If this commit is meant to create ABI stability, then I am afraid to say it won't change much, since it may likely break with new releases as I favor strong type safety over unchanging ABIs.

Versioning should be introduced though, to remind people of updating the interfaces, by requiring to pick the right ABI version, which will be checked upon loading the plugin.

Maybe I could use a checksum of the interface, a bit like it happens in ROS, which minimizes and standardizes interface definition code, and checksums it to generate a unique ID.

@sredna
Copy link
Author

sredna commented Apr 17, 2022

My point was not to lock down the ABI forever but in this case it is basically a free change. Exchange one BOOL for 32 flag bits that allows future expansion without breaking ABI.

Versioning should be introduced though, to remind people of updating the interfaces, by requiring to pick the right ABI version, which will be checked upon loading the plugin.

This will add a lot of extra pain for yourself.

Another option is to use cdecl instead of stdcall, that way you can append new parameters without breaking anything.

@maelh
Copy link
Owner

maelh commented Apr 18, 2022

Another option is to use cdecl instead of stdcall, that way you can append new parameters without breaking anything.
I'd still want the plugin authors to be aware of the new options/changed flags, so they are taken into account in the conversion, so a type error of some kind is desirable.

From the pure ABI side, you are right, it would just work. But I want it to fail explicitly, even if I was to implement a kind of Flags solution (for example using set in Delphi which are strongly typed, which would directly map to bit fields in C).

So the solution would need a bit more work, but thanks for the suggestion, I'll consider it in a possibly modified form.

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