-
-
Notifications
You must be signed in to change notification settings - Fork 107
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
enums,flags: Always analyze manual types #1164
enums,flags: Always analyze manual types #1164
Conversation
src/analysis/functions.rs
Outdated
let mut status = GStatus::Generate; | ||
let mut status = obj.status; | ||
for f in configured_functions.iter() { | ||
match f.status { | ||
GStatus::Ignore => continue 'func, | ||
GStatus::Manual => { | ||
status = GStatus::Manual; | ||
s => { | ||
status = s; | ||
break; | ||
} | ||
GStatus::Generate => (), | ||
} | ||
} |
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.
I'm not really sure what to do here. It is very counter-intuitive to have multiple configurations specifying a different mode. IMO we should assert that all statuses are equal. OTOH that might make it weird when using patterns to apply generic configuration, then later manual/ignore
a few specific cases?
This now breaks or continues on the first item anyway, and clippy will tell us that this doesn't loop.
Because of using the first item (instead of the first encounter of ignore = true
) this actually uncovers a little inconsistency in GStreamer's Element
where the "seek"
and "seek_simple"
functions first receive a bool_return_is_error
, then later down are reconfigured for manual = true
.
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 best course of action is probably to revert this change as it's not necessary to fix the issue described in the first commit, though semantics change slightly if a parent object would have been set to manual, but a function set to generate (which is not something we support now, but maybe something to recognize in the future).
In addition, the config parser should check and warn/fail on duplicate Ident::Name
.
When the surrounding type is ignored or manually implemented the underlying functions are never generated; make their `status` field reflect this case. This fixes a future case where flags and enums are going to be analyzed to have information available in documentation generation, despite being marked `manual`. These flags and enums collect all their imports in one object which have to be omitted for `manual` types, and an easy way to do that for the functions on types is to take advantage of the `status.need_generate()` check before function analysis starts analyzing and adding imports.
Docs generation is going to look at analysis data to know what variants and functions are available on enums/flags. External types (in the `manual = []` list) were previously omitted because of their status. This changes enums and flags to behave like objects and records which only bail if the type is entirely ignored, and perform a `need_generate()` check at `codegen` instead. Fixes: 50cee9e ("enums: Move rejection and glib imports from codegen to analysis") Fixes: e549975 ("Copypaste enum analysis and function generation to bitfield")
cda3ed9
to
68591f1
Compare
Found this ambiguity in gtk-rs/gir#1164.
Can confirm this fixes various warnings on the current docs like
|
Docs generation is going to look at analysis data to know what variants and functions are available on enums/flags. External types (in the
manual = []
list) were previously omitted because of their status. This changes enums and flags to behave like objects and records which only bail if the type is entirely ignored, and perform aneed_generate()
check atcodegen
instead.Fixes: 50cee9e ("enums: Move rejection and glib imports from codegen to analysis")
Fixes: e549975 ("Copypaste enum analysis and function generation to bitfield")
CC @bilelmoussaoui, we're going to need this for #1161.