Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like the branches are identical?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The only change is on the macos branch, i thought macos, windows and linux may be treated differently in the future so i kept three branches. if you think that that's not necessary i can remove the windows branch
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oops, I'm blind. I think it would be better to write the code in some different way, to avoid these copy-pasted lines. It's kinda difficult to see what's the difference, but I think it's like that just in diff. When I looked at the changes without diff, it was more clear.
This branching makes the generated bindings dependent on the environment they are generated, i.e. you can't use Mac to generate bindings that will work on Windows. Are you sure that's the desired behaviour for your use case?
I tried to run the tests locally and it's working without
.dylib
suffix. Can you test again without.dylib
suffix?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When I first tested it the loading failed complaining that the shared lib couldn't be found, it's been a minute since I did any interop in c# so I assumed it didn't handle extensions, as soon as I can I'll test it again and see if it works.
In case the lib still cannot be found would you be open to #if-ing the dllimport declarations to support platform specific extensions?
Something like
Obviously I'd add Linux too, I'm keeping it short here.
I'm writing on my phone so if it's messed up I'll fix the formatting in the morning.
Thank you for taking the time to review this 😊