🐛 xmlgen: avoid generating duplicate receive_*_changed methods
#1684
Conversation
CodSpeed Performance ReportMerging this PR will not alter performanceComparing Summary
|
antonok-edm
force-pushed
the
avoid-generating-signal-changed-collisions
branch
from
ddf72f5 to
b8ea635
Compare
zeenix
left a comment
zeenix
left a comment
There was a problem hiding this comment.
A few nitpicks resolved and we're good to merge. :)
antonok-edm
force-pushed
the
avoid-generating-signal-changed-collisions
branch
from
b8ea635 to
29cb297
Compare
antonok-edm
force-pushed
the
avoid-generating-signal-changed-collisions
branch
from
29cb297 to
b34e04d
Compare
When given an interface definition with a property named `Foo` and a signal named `FooChanged`, `zbus_xmlgen` would generate duplicate `receive_foo_changed` methods, causing compilation errors. In practice, the colliding `*Changed` signal is used to override the behavior of the property's implicit change signal, so it's preferable to use the overridden one and leave off the implicit rather than keeping it as a compilation error. Resolves `Case 2` of z-galaxy#231
antonok-edm
force-pushed
the
avoid-generating-signal-changed-collisions
branch
from
b34e04d to
7c8eee1
Compare
| signals.sort_by(|a, b| a.name().partial_cmp(&b.name()).unwrap()); | ||
|
|
||
| // collect signal method names for collision detection with implicit property change | ||
| // receivers |
There was a problem hiding this comment.
Missing . again. Sorry! I just like consistency too much. Although in these files I've allowed changes from contributors that missed . but it's inconsistent with rest of the code and even code here, so let's not add to that. 🙏 I'll add . to existing comments/docs in these files myself later.
| format!(" #[zbus(property, name = \"{}\")]", p.name()) | ||
|
|
||
| let prop_change_receiver_name = format!("{name}_changed"); | ||
| let signal_collision = changed_suffix_signals.get(&prop_change_receiver_name); |
There was a problem hiding this comment.
I liked the previous naming better. The variable isn't holding a collision but rather the Signal. The name makes it sound like it's a boolean so makes code slightly harder to follow/read.
| >; | ||
|
|
||
| /// State property | ||
| // Note: change signal is shadowed by `StateChanged` signal. |
There was a problem hiding this comment.
change signal->changed signal.- I just realized btw: wouldn't it be better if this was instead:
| // Note: change signal is shadowed by `StateChanged` signal. | |
| // Note: change signal is shadowed by `state_changed` signal method. |
? That would also make the xmlgen part simpler and you can also go back to HashSet but this time w/o the caveat of reallocation. 😉
| @@ -185,6 +186,11 @@ impl GenTrait<'_> { | |||
|
|
|||
| let mut signals = iface.signals().to_vec(); | |||
There was a problem hiding this comment.
btw, can't we just switch to be a HashMap instead? Wouldn't it make the code even simpler? 🤔
2 participants
Resolves
Case 2of #231I ran into this issue when trying to generate
zbusbindings for ModemManager, which has four such naming collisions. This resolves the issue there and should help for others who have commented on #231.