-
Notifications
You must be signed in to change notification settings - Fork 2.1k
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
[binding] remove binding when corresponding fabric removed #14487
[binding] remove binding when corresponding fabric removed #14487
Conversation
3ed24af
to
1c43f7b
Compare
PR #14487: Size comparison from d2fded9 to 1c43f7b Increases above 0.2%:
Increases (15 builds for cyw30739, efr32, k32w, linux, p6, qpg, telink)
Decreases (2 builds for linux, qpg)
Full report (16 builds for cyw30739, efr32, k32w, linux, p6, qpg, telink)
|
1c43f7b
to
11b9fe9
Compare
PR #14487: Size comparison from d2fded9 to 11b9fe9 Increases above 0.2%:
Increases (29 builds for cyw30739, efr32, esp32, k32w, linux, mbed, nrfconnect, p6, qpg, telink)
Decreases (2 builds for linux, qpg)
Full report (33 builds for cyw30739, efr32, esp32, k32w, linux, mbed, nrfconnect, p6, qpg, telink)
|
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.
This seems pretty reasonable, but also looks like it will conflict with #14303. I'd really prefer to review this after that lands and this is merged/rebased...
Since it's a long holiday in Shanghai and it will be some time before I can address the commentes in #14303, maybe we can merge this first? I'll rebase #14303 after I'm back in office next Monday. @bzbarsky-apple |
We can do that, but I thought 14303 was pretty close.... and importantly it will need a wholesale re-review on top of these changes. Which will delay it landing even more.... But whichever you prefer. |
/rebase |
Co-authored-by: Boris Zbarsky <[email protected]>
4b20e5c
to
7e8a072
Compare
PR #14487: Size comparison from 2294bcc to 7e8a072 Increases above 0.2%:
Increases (30 builds for cyw30739, efr32, esp32, k32w, linux, mbed, nrfconnect, p6, qpg, telink)
Decreases (2 builds for qpg)
Full report (34 builds for cyw30739, efr32, esp32, k32w, linux, mbed, nrfconnect, p6, qpg, telink)
|
Merging project-chip#14487 violated the API contract that was established in project-chip#14688, so we started failing tests. The two PRs had never been run through CI together.
Merging project-chip#14487 violated the API contract that was established in project-chip#14688, so we started failing tests. The two PRs had never been run through CI together.
Problem
Binding table entries are not correctly removed when fabric is removed.
Change overview
FabricTableDelegate
a linked list to support multiple handlers.Testing
Can observe binding removed log in the output.