-
-
Notifications
You must be signed in to change notification settings - Fork 81
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
Warning fixes #529
Warning fixes #529
Conversation
- const-qualified in the function declaration; const-qualification of parameters only has an effect in function definitions - Some [[nodiscard]]s added - default argument for `parse_port` which is a virtual function is not recommended, I changed it to two different functions - using `<cstdint>` instead of `<stdint.h>` - Adding `override` for virtual destructors - Small changes like comment missmatch fixes and useless "return" statement removal
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.
I strongly disagree with the change of uint8_t
into std::uint8_t
. You can verify that Node.js (written in C++) uses uint8_t
and not std::uint8_t
.
Everything else seems good and should be merged. Thanks.
There are some formatting changes that I am not going to comment upon (I only reviewed semantically relevant changes).
This is a good PR. |
formatting is done via clang-format v16.0.6 git ls-files | grep -E "\.(h|cpp)\$" | xargs clang-format -i And I'm going to change the |
[[nodiscard]]
s addedparse_port
which is a virtual function is not recommended, I changed it to two different functions<cstdint>
instead of<stdint.h>
override
for virtual destructors