- Notifications
You must be signed in to change notification settings - Fork3.8k
Segfault fix in freetype.get_version#3567
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
I don't know about this, until now it could show the version even if it wasn't initialized which kind of makes sense, so following that this should also just show the current version instead of raising an exception. However, this makes sense too, it just doesn't conform to the previous versions. |
@Matiiss here is what Although, I'm pretty sure that comment is wrong and those macros are the compiled version. That code still exists after the PR merge from above, but the linked version requires a |
oddbookworm commentedNov 15, 2022 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
In fact, here's the declaration of those macros in the |
But I think I have actually found a solution, so I'll add another commit |
src_c/_freetype.c Outdated
| PyErr_SetString(pgExc_SDLError, | ||
| "FreeType could not be initialized"); |
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.
how aboutPyExc_RuntimeError ?
MyreMylar commentedNov 19, 2022 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
Would it not be better just to do: Right now it seems like this would be leaking memory from initialising two versions of freetype and the one in |
@MyreMylar I see two routes:
With option 1, I understand that it's not the most ideal solution because of the extra initialization of freetype. But, I also would like to see this method behave similarly to the other getters, who don't need module initialization within pygame to work. Which is why I opted to pursue that route (and then forgetting to clean up after myself). Also considering that this method is intended to help diagnose problems with pygame, I think it would be bad form to throw an error or return a dummy value just because a module isn't initialized. I'd like to hear your thoughts about it. In the meantime, I'll go ahead and push a commit hopefully cleaning up properly (for some reason, FreeType2's docs are really hard to read for me) |
Thoughts--
|
It’s more of a “returning those lines back to the way they were before the PR that broke this function in the first place |
Uh oh!
There was an error while loading.Please reload this page.
Should we add a test that freetype.get_version() can be called before pygame.freetype.init? |
Uh oh!
There was an error while loading.Please reload this page.
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.
Good stuff, approved.
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.
LGTM 👍
Segfault fix in freetype.get_version
Fixed the segfault reported in#3564