-
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
Remove const for Concrete*Path.h members #10756
Remove const for Concrete*Path.h members #10756
Conversation
PR #10756: Size comparison from c4a4361 to 4628004 5 builds (for k32w, p6, telink)
17 builds (for efr32, linux, mbed, qpg)
12 builds (for esp32, nrfconnect)
|
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 is a failure of the goto pattern
no?
Const correctes is generally useful. Do we have instances in the code where these paths are used but we cannot get rid of the goto pattern?
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.
Re-marking as approved - the "changes requested" seems a bit harsh. It seems to be we already pass things as const &
for these paths so const correctness is preserved.
I would however like an aswer thogh: goto pattern forcing us to have c-style declarations and concessions in the code seems not right. I would much rather not use the goto exit.
Right. I have some other use case where I have So this is not only about the |
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.
Approving for other usages (i.e. not goto exit, but things like "current path" in a class).
Fast track reasoning: trivial change/refactor. |
As discussed on slack, I also needs it here : https://github.com/vivien-apple/connectedhomeip-1/blob/Tests_Node/examples/chip-tool/templates/partials/test_cluster.zapt#L112 I can workaround that by manually storing all those elements and making comparison against each but that sounds painful... |
Problem
The following code does not work because of
Concrete*Path
members beeingconst
.Change overview
const
for membersTesting
There is no behaviour changed afaict and I have some local changes where I need this so that's how I have checked that it works.