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

Uint flags in sys #577

Merged
merged 4 commits into from Apr 3, 2018

Conversation

Projects
None yet
4 participants
@EPashkin
Member

EPashkin commented Apr 2, 2018

Part of #575

@sdroege Please, check changes in gstreamer, especially in GST_BUFFER_COPY_ALL and other constants based on flags

cc @GuillaumeGomez,, @tmiasko

@sdroege

This comment has been minimized.

Show comment
Hide comment
@sdroege

sdroege Apr 2, 2018

Member

The results look good to me but I still have to test it with gstreamer later. Thanks!

Member

sdroege commented Apr 2, 2018

The results look good to me but I still have to test it with gstreamer later. Thanks!

@tmiasko

This comment has been minimized.

Show comment
Hide comment
@tmiasko

tmiasko Apr 2, 2018

Contributor

Looks fine to me as well.

BTW, do you know if this cast val as u32 is necessary and if so why?

Contributor

tmiasko commented Apr 2, 2018

Looks fine to me as well.

BTW, do you know if this cast val as u32 is necessary and if so why?

@EPashkin

This comment has been minimized.

Show comment
Hide comment
@EPashkin

EPashkin Apr 2, 2018

Member

IMHO We parse string value as i64 first to make sure that it fully parsed (in case introspection error etc),
and then make it unsigned.
Seems this need be as c_uint currently as we changed flags type from u32 long ago.

Member

EPashkin commented Apr 2, 2018

IMHO We parse string value as i64 first to make sure that it fully parsed (in case introspection error etc),
and then make it unsigned.
Seems this need be as c_uint currently as we changed flags type from u32 long ago.

@EPashkin

This comment has been minimized.

Show comment
Hide comment
@EPashkin

EPashkin Apr 2, 2018

Member

.. but this will need add libc as dependence to gir. So I not sure that about doing this change

Member

EPashkin commented Apr 2, 2018

.. but this will need add libc as dependence to gir. So I not sure that about doing this change

@tmiasko

This comment has been minimized.

Show comment
Hide comment
@tmiasko

tmiasko Apr 2, 2018

Contributor

The commit that introduced this described this as "convert negative bitflags members". Now I see one example in GLib, G_LOG_LEVEL_MASK is -4, but that's about it.

Contributor

tmiasko commented Apr 2, 2018

The commit that introduced this described this as "convert negative bitflags members". Now I see one example in GLib, G_LOG_LEVEL_MASK is -4, but that's about it.

@sdroege

This comment has been minimized.

Show comment
Hide comment
@sdroege

sdroege Apr 2, 2018

Member

Now I see one example in GLib, G_LOG_LEVEL_MASK is -4, but that's about it.

That's just a shortcut and exactly what I meant in the other issue about signedness being basically meaningless in GLib-style C. That's 0xfffffffc, i.e. all bits set except for the lowest 2 (which are G_LOG_FLAG_RECURSION and G_LOG_FLAG_FATAL and have a special meaning compared to all others).

Member

sdroege commented Apr 2, 2018

Now I see one example in GLib, G_LOG_LEVEL_MASK is -4, but that's about it.

That's just a shortcut and exactly what I meant in the other issue about signedness being basically meaningless in GLib-style C. That's 0xfffffffc, i.e. all bits set except for the lowest 2 (which are G_LOG_FLAG_RECURSION and G_LOG_FLAG_FATAL and have a special meaning compared to all others).

@GuillaumeGomez

This comment has been minimized.

Show comment
Hide comment
@GuillaumeGomez

GuillaumeGomez Apr 2, 2018

Member

That's quite the removal of code. I'm all for it!

Member

GuillaumeGomez commented Apr 2, 2018

That's quite the removal of code. I'm all for it!

w,
"{}pub const {}: {} = {};",
comment, constant.c_identifier, type_, value
));

This comment has been minimized.

@EPashkin

EPashkin Apr 3, 2018

Member

There will be problem with this for negative const's like G_LOG_LEVEL_MASK,
so maybe better restore special case for Bitfield with same action as in bitfield generation?

@EPashkin

EPashkin Apr 3, 2018

Member

There will be problem with this for negative const's like G_LOG_LEVEL_MASK,
so maybe better restore special case for Bitfield with same action as in bitfield generation?

This comment has been minimized.

@sdroege

sdroege Apr 3, 2018

Member

ack

@sdroege

This comment has been minimized.

@tmiasko

tmiasko Apr 3, 2018

Contributor

This was actually never handled with casting, and G_LOG_LEVEL_MASK is not
generated by this code but rather as a part of generate_bitfields, so there is
no pressing need to fix this as a part of this PR, but in general it would be
nice for consistency.

@tmiasko

tmiasko Apr 3, 2018

Contributor

This was actually never handled with casting, and G_LOG_LEVEL_MASK is not
generated by this code but rather as a part of generate_bitfields, so there is
no pressing need to fix this as a part of this PR, but in general it would be
nice for consistency.

@sdroege

This comment has been minimized.

Show comment
Hide comment
@sdroege

sdroege Apr 3, 2018

Member

@EPashkin Looks all good with GStreamer and seems to speed up compilation even.

Member

sdroege commented Apr 3, 2018

@EPashkin Looks all good with GStreamer and seems to speed up compilation even.

@EPashkin

This comment has been minimized.

Show comment
Hide comment
@EPashkin

EPashkin Apr 3, 2018

Member

@sdroege Thanks

Member

EPashkin commented Apr 3, 2018

@sdroege Thanks

@EPashkin

This comment has been minimized.

Show comment
Hide comment
@EPashkin

EPashkin Apr 3, 2018

Member

Updated generating constants, based on bitfield

Member

EPashkin commented Apr 3, 2018

Updated generating constants, based on bitfield

@EPashkin EPashkin merged commit 6855214 into gtk-rs:master Apr 3, 2018

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@EPashkin EPashkin deleted the EPashkin:uint_flags_in_sys branch Apr 3, 2018

vhdirk pushed a commit to vhdirk/gir that referenced this pull request Jul 6, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment