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

fixed buggy gnustl implementation of std::vector as well as a false positive for types mismatch #3

Merged
merged 1 commit into from
Aug 16, 2024

Conversation

AngelDev06
Copy link
Contributor

other small changes I did include:

  • moving holy_shit at the bottom of the generated file (it requires definitions for some of the types and I don't see why this would cause any problems)
  • the check for non-existing pads actually returns true even when pad is explicitly set to 0 so it's safer to continue than break

@SpaghettDev
Copy link
Owner

im sorry but I dont agree with a lot of your changes;

  1. I would rather have the includes in the header than manually putting it in the codegenned header (also makes your LSP not go berserk when you open the file)
  2. same reason why I manually forward declare the classes used in the stl_types.hpp header, I would rather be able to open the file in vscode than save 20 lines and be greeted with 3 billion undeclared identifiers
  3. the change to the non-existing pads isnt really a fix IMO, I could try to find a better fix even though I never encountered this issue.
    If you feel like changing your PR I will gladly merge, if not i can manually implement the stuff I agree with

@AngelDev06
Copy link
Contributor Author

  1. I would rather have the includes in the header than manually putting it in the codegenned header (also makes your LSP not go berserk when you open the file)

The includes are still in the header, I did not remove them.

The reason why I hardcoded them to the script is because I moved the holy_shit struct to the bottom and so the includes were moved too. So I had to hardcode them to the top in order to fix errors with gd classes that depend on them.

This is not an issue for android as the gnustl header has the includes needed and is included before the gd classes.

  1. same reason why I manually forward declare the classes used in the stl_types.hpp header, I would rather be able to open the file in vscode than save 20 lines and be greeted with 3 billion undeclared identifiers

I can bring back the forward declarations, but mind that the struct also requires a few definitions (which is the reason why I moved it to the bottom).

  1. the change to the non-existing pads isnt really a fix IMO, I could try to find a better fix even though I never encountered this issue.

Yeah it's not a fix actually, I may have been using an older commit of the bindings repo which did not distinguish between 32 bit android and 64 as pybroma expects.

Nevertheless I think it's still better to continue with the rest of the members even if the pads are not implemented for the current platform (I will make sure to update the comment).

Also yes this fixes the false positive for types mismatch referenced in the issue.

@SpaghettDev
Copy link
Owner

Nevertheless I think it's still better to continue with the rest of the members even if the pads are not implemented for the current platform (I will make sure to update the comment).

Why so? I would think a user would rather have an, albeit messed up, member by offset (usually this[1].m_somemember + someoffset) than something completely wrong.

also moved holy_shit to the bottom as it needs some definitions
@AngelDev06
Copy link
Contributor Author

Alright I restored the old contents of the class builder.

@SpaghettDev
Copy link
Owner

I'll rework the STL header includes, because it currently sucks and this PR made me realize so. anyway thanks for fixing epic bromaida bug number 5632 👍

@SpaghettDev SpaghettDev merged commit dc8860c into SpaghettDev:master Aug 16, 2024
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