-
Notifications
You must be signed in to change notification settings - Fork 685
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
Add Layer Build and Validation for DoIP (Diagnostic over IP) Support #1655
base: dev
Are you sure you want to change the base?
Changes from 12 commits
a50cf68
703b650
7aee737
7966b06
d4658d7
dd26c31
2ed8c0a
0d52dbf
b317bf6
3ba3c71
7822458
3c5dc4d
7896a9e
c159be9
14bc5a5
0545afe
14c9842
c4b8baa
326ae50
9d97fce
031b36c
929287b
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. IMO, it would be better to have the enum to string maps be converted to functions with switch statements. It would allow to expose a cleaner and uniform public API ( The functions can then be folded into There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thanks for your feedback, @Dimi1010 In my humble opinion, unordered_map looks much cleaner and more declarative compared to switch statements, which can sometimes feel cluttered with redundant return or break statements. I’ve also ensured that edge cases, like missing keys, are handled appropriately whenever I use these maps. To be honest, I haven’t focused much on the potential performance impact, as I believe the effect with these small enums would be negligible, though I understand it might require additional effort. That said, if the difference in performance isn’t significant, I think we can keep it as it is. However, if you strongly feel this change is necessary, I’ll plan to prepare a fix over the weekend. Thank you for your understanding! There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Sure. Not necessarily a strong opinion on the Something else that occurred to me. The maps are declared as Additionally, Are the maps supposed to be part of the public API or to be used only through the methods in
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @Dimi1010 , thank you for your feedback! |
Large diffs are not rendered by default.
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.
Why update this file?
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.
same thing as my previous comment, I only removed some suppress-checks detected as incorrect