Skip to content
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

lib: Discourage usage of deprecated data structures #16140

Merged
merged 1 commit into from
Jul 10, 2024

Conversation

donaldsharp
Copy link
Member

Put some verbiage in place to warn people that we
are actively discouraging new development that uses an older data structure.

Copy link
Contributor

@mjstapp mjstapp left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, thanks, great idea

Copy link
Contributor

@choppsv1 choppsv1 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The bsd queues are typesafe though, right?

@donaldsharp
Copy link
Member Author

I would prefer that we stop using the BSD variants and get them removed from the repository in the long term.

@choppsv1
Copy link
Contributor

choppsv1 commented Jun 1, 2024

I would prefer that we stop using the BSD variants and get them removed from the repository in the long term.

Figured, but maybe we shouldn't refer to using the type safe variants instead when talking about moving away from type safe variants :) Maybe call them FRR native type safe variants (or maybe better name them explicitly?)

@ton31337
Copy link
Member

ton31337 commented Jun 2, 2024

Agree with Donald, BSD variants are kinda OK, but we need to sync them (same as we do for some Linux headers...).

@choppsv1
Copy link
Contributor

choppsv1 commented Jun 3, 2024

Agree with Donald, BSD variants are kinda OK, but we need to sync them (same as we do for some Linux headers...).

This ship has probably already sailed, but ...

The problem with our type safe macros, IMO, is that when you use an IDE, the FRR type safe stuff is hyper obfuscated (and in general I find the generated macro names somewhat non-noticeable as well), whenever you "Go to definition" to see what a function call is doing (e.g., dplane_ctx_list_add_tail) they all lead you back to a single MACRO (e.g., DECLARE_DLIST(dplane_ctx_list...), and not to the code tha actually adds something to the tail of a list.

The BSD macros are just as type-safe but much more obvious (and obviously named e.g., TAILQ_INSERT_TAIL() and you actually are taken to the code that is implements the operation. I think that's valuable.

@donaldsharp
Copy link
Member Author

Fixing our typesafe code is a bit orthogonal to what I am trying to do here. I would just like to let people know that they should think about using something different.

@choppsv1
Copy link
Contributor

choppsv1 commented Jun 4, 2024

Fixing our typesafe code is a bit orthogonal to what I am trying to do here. I would just like to let people know that they should think about using something different.

My original comment was that referring to "typesafe" meaning "look elsewhere" in the openbsd-queue.h file is imprecise since the things defined in that header can be called "typesafe" and you're trying to point people away from them. :)

FWIW, I absolutely support putting the comments in the hash.h and linklist.h headers, without modification.

Now, my personal opinion is that we shouldn't put these comments in the openbsd header files, but if that's too big an ask perhaps at least we don't suggest people convert existing use away from them?

* of this data structure, please consider just converting the data
* structure to one of the typesafe data structures instead.
*/

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For the BSD variants (only) could we instead say:

If you are reading this file in an effort to add a new queue/tree
structure, please consider using the FRR native versions found in
lib/typesafe.h as an approved alternative.

@eqvinox
Copy link
Contributor

eqvinox commented Jun 19, 2024

just to record, the consensus on yesterday's community call was to add some additional text on the BSD datastructures along the lines of However, among converting datastructures, the BSD ones are the lowest priority / should be converted last. They are already typesafe and use inline linking nodes, so the only gain is consistency. Please convert uses of linklist.h and hash.h first.

Put some verbiage in place to warn people that we
are actively discouraging new development that uses
an older data structure.

Signed-off-by: Donald Sharp <[email protected]>
@donaldsharp donaldsharp force-pushed the linklist_discouragement branch from fe4e184 to 69b36cd Compare June 19, 2024 11:50
@donaldsharp
Copy link
Member Author

updated

@eqvinox eqvinox merged commit ebf05b4 into FRRouting:master Jul 10, 2024
9 of 10 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants