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

Provide TypePrinters with more context #1788

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

Saalvage
Copy link
Contributor

I am absolutely uncertain about this pull request, the underlying mechanisms for this library functionality is still an enigma to me.

What I seek to achieve:
I am still working on the SDL3 bindings. Some of the classes in SDL3 contain fields which are of primitive types (uint32_t) but actually represent enum types. C enums being weakly typed bla bla bla. I would like to map these specific fields (which I hand pick) to the corresponding enum types. For this purpose I think TypeMappers are the tool of choice. I have already established similar functionality for function parameters of primitive types that represent enums. I simply collect all Parameters and their corresponding Type in a dictionary and check that when marshalling.

Trying to replicate this same approach for fields has presented me with the problem that the context that is provided to a TypeMapper is not able to decide whether it's a field that should be mapped. There is very minimal information given, no ability to derive a corresponding class or anything of that sort.

Modifying the library to make what I desire work is fairly trivial. I check for the property in CSharpSignatureType (not the field!) and for the field in the marshalling methods (it's the only thing that's provided respectively).

Alternative approaches I've considered:

  • Changing the field's type in PreProcess and dealing with the marshalling properly via a TypeMapper for the enum
    • Causes a stack overflow for some reason
  • Changing the property's and internal classes field type in PostProcess
    • I can't seem to be able to find the inner __Internal class

Now, the considerations for this pull request:

  • The Parameter field does not actually seem to represent a concrete method parameter, it can be different things for different contexts. Sometimes it's not set at all, other times there's a degenerate version newly created from a different Parameter that has a different naming scheme. So all in all I feel like this PR is not line with the original design intent here (because I currently don't think I understand it).
  • While I only require Field on TypePrinter and Property on TypePrinterContext I have also decided to include both of them in both classes, as well as an additional Method field that could be helpful when trying to recreate the desired functionality for return types.
  • TypePrinterContext's Field property goes unset.
  • Variable is another often-seen class that could be considered for addition. I have decided to exclude it because I cannot see a direct use for it.

I actually hope that what I seek to accomplish can be accomplished using existing library mechanisms, I guess this would've been more fit for an issue asking for help, but I think it might be worth presenting the code here for consideration anyways.

If this PR were to be considered for merging it would definitely require a second closer look by someone with a deeper understanding of the library. It's highly probably that I missed spots for potentially providing the new fields and other times I have provided them needlessly at the very least.

To conclude here's a minimal example that demonstrates what I seek to achieve:

#include <cstdint>

typedef enum {
    A = 0, B = 1, C = 2
} MyEnum;

class MyClass {
public:
    uint32_t member_enum;
    uint32_t member_normal;
};

I want member_enum to have type MyEnum on the C# side and member_normal to remain as uint32_t/uint.

@tritao
Copy link
Collaborator

tritao commented Oct 22, 2023

I think you're finding it a bit awkward to use type maps because fundamentally they are used for mapping types, whereas you want to map a declaration.

I actually hope that what I seek to accomplish can be accomplished using existing library mechanisms, I guess this would've been more fit for an issue asking for help, but I think it might be worth presenting the code here for consideration anyways.

I added the concept of a declaration map before, but it's still very half baked, seems like it's only implemented for the C++ generator. I probably used it for some JS generation stuff I was working on.

public abstract class DeclMap

public class DeclMapDatabase : IDeclMapDatabase

if (Context.DeclMaps.FindDeclMap(method, out DeclMap declMap))

So one possible approach might be extend this. We could even add a Declaration.DeclMap property, so you could just assign it per decl.

Though I think my idea with the DeclMap feature was more to be able to generate totally custom bindings per decl, whereas what you want is more to slightly hook into the existing generation logic. So not sure if its worth it extending for what you need, maybe since you also need to map how marshaling is done, TypeMap already provides those things, so could be better base to build from.

Anyway take a look and maybe it can give you some ideas.

If this PR were to be considered for merging it would definitely require a second closer look by someone with a deeper understanding of the library. It's highly probably that I missed spots for potentially providing the new fields and other times I have provided them needlessly at the very least.

I think the changes in this PR look fine, development in this library has always been driven by the needs of development of new bindings, so even if this isn't totally complete, we can always improve it in the future.

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.

None yet

2 participants