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

Assert that GDK is initialized in constructors and free functions #82

Merged
merged 3 commits into from Nov 17, 2015

Conversation

Projects
None yet
2 participants
@gkoz
Member

gkoz commented Nov 16, 2015

The first stab at fixing gtk-rs/gtk#188

unsafe {
ffi::gdk_set_program_class(program_class.to_glib_none().0)
}
}
pub fn flush() {
assert_initialized_main_thread!();

This comment has been minimized.

@GuillaumeGomez

GuillaumeGomez Nov 16, 2015

Member

This is exactly what I was afraid of. We now have to call the macro everywhere. :-/

@GuillaumeGomez

GuillaumeGomez Nov 16, 2015

Member

This is exactly what I was afraid of. We now have to call the macro everywhere. :-/

@GuillaumeGomez

This comment has been minimized.

Show comment
Hide comment
@GuillaumeGomez

GuillaumeGomez Nov 16, 2015

Member

Thanks for this. I'm really not comfortable here since we might forget places from where it could need it... We already told about this, so let's say this is a temporary solution while we try to figure out something better.

Member

GuillaumeGomez commented Nov 16, 2015

Thanks for this. I'm really not comfortable here since we might forget places from where it could need it... We already told about this, so let's say this is a temporary solution while we try to figure out something better.

@gkoz

This comment has been minimized.

Show comment
Hide comment
@gkoz

gkoz Nov 16, 2015

Member

I'll look into making a script, that would keep us from forgetting to put the assert everywhere it's needed. Adding this to gtk by hand is a nonstarter anyway. ;)

Member

gkoz commented Nov 16, 2015

I'll look into making a script, that would keep us from forgetting to put the assert everywhere it's needed. Adding this to gtk by hand is a nonstarter anyway. ;)

@GuillaumeGomez

This comment has been minimized.

Show comment
Hide comment
@GuillaumeGomez

GuillaumeGomez Nov 16, 2015

Member

I absolutely agree with you, it'd take an eternity or two to do it. Can't we add it to one of the macros which generates structs (widgets and others) ? Or do you want an external script ?

Member

GuillaumeGomez commented Nov 16, 2015

I absolutely agree with you, it'd take an eternity or two to do it. Can't we add it to one of the macros which generates structs (widgets and others) ? Or do you want an external script ?

@gkoz

This comment has been minimized.

Show comment
Hide comment
@gkoz

gkoz Nov 16, 2015

Member

I'm afraid there's just no centralized place to put this. The generator will need to be adjusted to emit the assert of course but this won't cover everything. I'll see if my favorite tool, PCRE, is going to be helpful here.

Member

gkoz commented Nov 16, 2015

I'm afraid there's just no centralized place to put this. The generator will need to be adjusted to emit the assert of course but this won't cover everything. I'll see if my favorite tool, PCRE, is going to be helpful here.

@gkoz

This comment has been minimized.

Show comment
Hide comment
@gkoz

gkoz Nov 16, 2015

Member

Anyway don't merge this yet, I forgot to reexport set_initialized.

Member

gkoz commented Nov 16, 2015

Anyway don't merge this yet, I forgot to reexport set_initialized.

@GuillaumeGomez

This comment has been minimized.

Show comment
Hide comment
@GuillaumeGomez

GuillaumeGomez Nov 16, 2015

Member

Just ping me when it's done then. :)

Member

GuillaumeGomez commented Nov 16, 2015

Just ping me when it's done then. :)

@gkoz

This comment has been minimized.

Show comment
Hide comment
@gkoz

gkoz Nov 17, 2015

Member

Added a script to check any safe non-method fn has an assertion or skips one explicitly. This has actually caught several functions I've missed previously so you were absolutely right to be worried.

Member

gkoz commented Nov 17, 2015

Added a script to check any safe non-method fn has an assertion or skips one explicitly. This has actually caught several functions I've missed previously so you were absolutely right to be worried.

@@ -10,10 +10,10 @@ use ffi::{self, GdkRGBA};
use super::{Pixbuf, Window};
use cairo::{Context, RectangleInt};
//pub fn create_region_from_surface() { }
//pub fn create_region_from_surface()

This comment has been minimized.

@GuillaumeGomez

GuillaumeGomez Nov 17, 2015

Member

Your script deletes "{ }" too ? Is it a vonlontary behavior ?

@GuillaumeGomez

GuillaumeGomez Nov 17, 2015

Member

Your script deletes "{ }" too ? Is it a vonlontary behavior ?

This comment has been minimized.

@gkoz

gkoz Nov 17, 2015

Member

I had to delete those to avoid false positives.

@gkoz

gkoz Nov 17, 2015

Member

I had to delete those to avoid false positives.

This comment has been minimized.

@gkoz

gkoz Nov 17, 2015

Member

The included script doesn't attempt to modify the files it only prints any offending functions.

@gkoz

gkoz Nov 17, 2015

Member

The included script doesn't attempt to modify the files it only prints any offending functions.

This comment has been minimized.

@GuillaumeGomez

GuillaumeGomez Nov 17, 2015

Member

Oh okay, I thought it did both. I didn't look enough.

@GuillaumeGomez

GuillaumeGomez Nov 17, 2015

Member

Oh okay, I thought it did both. I didn't look enough.

@GuillaumeGomez

This comment has been minimized.

Show comment
Hide comment
@GuillaumeGomez

GuillaumeGomez Nov 17, 2015

Member

Also, what happens if the user quits the gtk_main() function and then relaunch it ? I'm not sure of what will happen in a C code, I should test later.

Member

GuillaumeGomez commented Nov 17, 2015

Also, what happens if the user quits the gtk_main() function and then relaunch it ? I'm not sure of what will happen in a C code, I should test later.

@gkoz

This comment has been minimized.

Show comment
Hide comment
@gkoz

gkoz Nov 17, 2015

Member

gtk_main doesn't perform any destruction or significant initialization (it it unsafe to call gtk_main_quit outside the main loop so I'm going to add a check there).

Member

gkoz commented Nov 17, 2015

gtk_main doesn't perform any destruction or significant initialization (it it unsafe to call gtk_main_quit outside the main loop so I'm going to add a check there).

@GuillaumeGomez

This comment has been minimized.

Show comment
Hide comment
@GuillaumeGomez

GuillaumeGomez Nov 17, 2015

Member

Always wonderful little annoying tricks to handle... Poor us :'(

Member

GuillaumeGomez commented Nov 17, 2015

Always wonderful little annoying tricks to handle... Poor us :'(

@gkoz

This comment has been minimized.

Show comment
Hide comment
@gkoz

gkoz Nov 17, 2015

Member

@GuillaumeGomez okay, this one seems ready, it has to go in before the gtk counterpart.

Member

gkoz commented Nov 17, 2015

@GuillaumeGomez okay, this one seems ready, it has to go in before the gtk counterpart.

gkoz added some commits Nov 16, 2015

Assert that GDK is initialized in constructors and free functions
Methods and functions, that take GDK objects as arguments, don't need this check.
Add a script that ensures the asserts are present or explicitly skipped.
@GuillaumeGomez

This comment has been minimized.

Show comment
Hide comment
@GuillaumeGomez

GuillaumeGomez Nov 17, 2015

Member

Okay, I merge. Thanks !

Member

GuillaumeGomez commented Nov 17, 2015

Okay, I merge. Thanks !

GuillaumeGomez added a commit that referenced this pull request Nov 17, 2015

Merge pull request #82 from gkoz/safety
Assert that GDK is initialized in constructors and free functions

@GuillaumeGomez GuillaumeGomez merged commit e30dc51 into gtk-rs:master Nov 17, 2015

1 of 2 checks passed

continuous-integration/travis-ci/pr The Travis CI build is in progress
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment