List API add const and return object on *_del
#4723
Conversation
💚 Basic BGPD CI results: SUCCESS, 0 tests failedResults table
For details, please contact louberger |
Continuous Integration Result: SUCCESSFULCongratulations, this patch passed basic tests Tested-by: NetDEF / OpenSourceRouting.org CI System CI System Testrun URL: https://ci1.netdef.org/browse/FRR-FRRPULLREQ-8430/ This is a comment from an automated CI system. Warnings Generated during build:Checkout code: Successful with additional warningsWarnings Generated during build:Debian 10 amd64 build: Successful with additional warningsDebian Package lintian failed for Debian 10 amd64 build: CLANG Static Analyzer Summary
No Changes in Static Analysis warnings compared to base1 Static Analyzer issues remaining.See details at |
🚧 Basic BGPD CI results: Partial FAILURE, 1 tests failedResults table
For details, please contact louberger |
Continuous Integration Result: SUCCESSFULCongratulations, this patch passed basic tests Tested-by: NetDEF / OpenSourceRouting.org CI System CI System Testrun URL: https://ci1.netdef.org/browse/FRR-FRRPULLREQ-8435/ This is a comment from an automated CI system. Warnings Generated during build:Debian 10 amd64 build: Successful with additional warningsDebian Package lintian failed for Debian 10 amd64 build: CLANG Static Analyzer Summary
No Changes in Static Analysis warnings compared to base1 Static Analyzer issues remaining.See details at |
I imagine its spurious but I don't have access to look at it... |
💚 Basic BGPD CI results: SUCCESS, 0 tests failedResults table
For details, please contact louberger |
lib/typerb.c
Outdated
| int (*cmpfn)( | ||
| const struct typed_rb_entry *a, | ||
| const struct typed_rb_entry *b)) | ||
| struct rb_entry *typed_rb_find(const struct rbt_tree *rbt, |
There was a problem hiding this comment.
NAK on returning a non-const rb_entry from a const rbt_tree
As much as I like putting const on things, this is a situation where it doesn't really work out.
There was a problem hiding this comment.
Sorry, can you elaborate a bit more on why this is an issue? Is actually a code issue or just an API nit because I can't produce a test case where it is breaking stuff.
Also, is it just a problem with the _find() api or do _first() and others need to be changed back as well?
There was a problem hiding this comment.
The problem is that const on the data structure head doesn't have clearly defined semantics.
- does it mean that I'm not supposed to allocate/free the head? That doesn't work because the DS might need to change something in the head on insertion, so cross that out...
- does it mean that I'm not supposed to change the DS but can do with the items what I want? That's kinda weird... why is the DS
constbut not the items? - does it mean that I'm not supposed to change anything? Then the items need to be const too...
Even if we agree on a semantic to attach, I'd argue that this is non-obvious enough to make sticking const on the DS head useless.
(My comment applies to all functions, _first & co too.)
So — what's your use case?
There was a problem hiding this comment.
I didn't think this was a problem when I saw it - though I think I understand your question. The use-case for const collection but not const item would be that I want to modify an item in a collection, but not the collection itself.
There was a problem hiding this comment.
ok, yea that makes sense. Thanks for taking the time to explain that.
I mostly decided whether my const decisions were appropriate by looking at the linux implementation: https://github.com/torvalds/linux/blob/master/include/linux/rbtree.h
But I definitely agree it in non-obvious, so I will remove it.
I will need to remove it in the other API's in typesafe.h as well,
The new list api did not implement the `*_del` endpoint as it was described in the docs here: http://docs.frrouting.org/projects/dev-guide/en/latest/lists.html#c.Z_del This patch implements the endpoints to return the object deleted if found, otherwise NULL for all but the atomic lists. The atomic list `*_del` code is marked as TODO and will remain undefined for now. Signed-off-by: Stephen Worley <sworley@cumulusnetworks.com>
Add some asserts where `list_del()` is called to verify they object was found when it was deleted. Signed-off-by: Stephen Worley <sworley@cumulusnetworks.com>
Checkpatch was complaining about the lack of identifiers here, so added some. Signed-off-by: Stephen Worley <sworley@cumulusnetworks.com>
Add const to the datastructure being passed to the new list APIs for the `_count()` calls. Signed-off-by: Stephen Worley <sworley@cumulusnetworks.com>
|
Force-pushed to rebase and remove |
💚 Basic BGPD CI results: SUCCESS, 0 tests failedResults table
For details, please contact louberger |
Continuous Integration Result: SUCCESSFULCongratulations, this patch passed basic tests Tested-by: NetDEF / OpenSourceRouting.org CI System CI System Testrun URL: https://ci1.netdef.org/browse/FRR-FRRPULLREQ-8486/ This is a comment from an automated CI system. Warnings Generated during build:Debian 10 amd64 build: Successful with additional warningsDebian Package lintian failed for Debian 10 amd64 build: CLANG Static Analyzer Summary
No Changes in Static Analysis warnings compared to base1 Static Analyzer issues remaining.See details at |
Add const where appropriate to the new list APIs.
We were not implementing the
*_del()API according to our docs here: http://docs.frrouting.org/projects/dev-guide/en/latest/lists.html#c.Z_delThis patch implements
*_delappropriately for all but the atomic list API. For that, it will return the object that was passed, but noNULLreturn is done if its not found in the list. This will remain a TODO for someone who has a better understanding of the atomic code path.