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.

Sign up for GitHub

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

Jump to bottom

net: make the list of known message types a compile time constant #29421

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

net: make the list of known message types a compile time constant #29421

wants to merge 2 commits into from
+80 −131

Conversation

vasild
Copy link
Contributor

@vasild vasild commented Feb 11, 2024 •
edited

Turn the std::vector to std::array because it is cheaper and allows us to have the number of the messages as a compile time constant: ALL_NET_MESSAGE_TYPES.size() which can be used in future code to build other std::arrays with that size.

This change is part of #29418 but it makes sense on its own and would be good to have it, regardless of the fate of #29418. Also, if this is merged, that would reduce the size of #29418, thus the current standalone PR.

@DrahtBot
Copy link
Contributor

DrahtBot commented Feb 11, 2024 •
edited

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Code Coverage

For detailed information about the code coverage, see the test coverage report.

Reviews

See the guideline for information on the review process.

Type Reviewers
Concept NACK maflcko
Concept ACK epiccurious, Empact
Stale ACK willcl-ark

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

Conflicts

Reviewers, this pull request conflicts with the following ones:

  • #29418 (rpc: provide per message stats for global traffic via new RPC 'getnetmsgstats' by vasild)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

@DrahtBot DrahtBot added the P2P label Feb 11, 2024
@epiccurious
Copy link

Concept ACK 2e312ea.

@DrahtBot DrahtBot mentioned this pull request Feb 12, 2024
Closed
willcl-ark
willcl-ark approved these changes Feb 14, 2024
View reviewed changes
Copy link
Member

@willcl-ark willcl-ark left a comment

Choose a reason for hiding this comment

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

ACK 2e312ea

This is a clean tidy-up, and as mentioned useful for #29418 but stands on it's own (as a smalle improvement) too.

Left one req for comment update if touched again.

src/protocol.cpp Show resolved Hide resolved
@vasild vasild force-pushed the message_types_compile_constant branch from 2e312ea to 808fe4e Compare February 14, 2024 16:45
@vasild
Copy link
Contributor Author

vasild commented Feb 14, 2024

2e312ea909...808fe4ef80: update comment, see #29421 (comment)

maflcko
maflcko reviewed Feb 15, 2024
View reviewed changes
src/net.cpp Outdated Show resolved Hide resolved
src/protocol.h Outdated Show resolved Hide resolved
src/test/fuzz/p2p_transport_serialization.cpp Show resolved Hide resolved
@vasild vasild mentioned this pull request Feb 15, 2024
Merged
@vasild vasild force-pushed the message_types_compile_constant branch from 808fe4e to c29350f Compare February 15, 2024 14:47
@DrahtBot DrahtBot mentioned this pull request Feb 15, 2024
Draft
@vasild vasild force-pushed the message_types_compile_constant branch from c29350f to c7c613c Compare February 15, 2024 18:58
Empact
Empact reviewed Feb 27, 2024
View reviewed changes
src/protocol.h Show resolved Hide resolved
Empact
Empact reviewed Feb 27, 2024
View reviewed changes
src/protocol.h Outdated Show resolved Hide resolved
@Empact
Copy link
Member

Empact commented Feb 27, 2024

Concept ACK

@maflcko
Copy link
Member

maflcko commented Feb 28, 2024

Seems fine, but NACK on the approach. This switches a vector of std::string objects (which is type-safe) to an array of raw pointers (not type-safe). This has already lead to a bug in this pull request, and is asking for more bugs to happen in the future, so I don't think it is worth it as-is.

@vasild vasild force-pushed the message_types_compile_constant branch from c7c613c to fd6a481 Compare February 28, 2024 14:37
@vasild
Copy link
Contributor Author

vasild commented Feb 28, 2024

c7c613c414...fd6a481995: rename g_all_net_message_types to ALL_NET_MESSAGE_TYPES as per #29421 (comment)

@vasild vasild force-pushed the message_types_compile_constant branch from fd6a481 to 546d132 Compare February 28, 2024 16:57
@vasild
Copy link
Contributor Author

vasild commented Feb 28, 2024

@maflcko good point! Changed from const char* to std::string in fd6a481995...546d13274d (can't have constexpr with std::string, thus dropped the constexpr, it was never the point of this PR).

@vasild
net: make the list of known message types a compile time constant
2fa9de0 
Turn the `std::vector` to `std::array` because it is cheaper and
allows us to have the number of the messages as a compile time
constant: `ALL_NET_MESSAGE_TYPES.size()` which can be used in
future code to build other `std::array`s with that size.
@vasild
protocol: make message types constexpr
b3efb48 
As a side effect the names of the constants do not have to be
repeated in `src/protocol.cpp`.
@vasild vasild force-pushed the message_types_compile_constant branch from 546d132 to b3efb48 Compare February 28, 2024 17:04
@vasild
Copy link
Contributor Author

vasild commented Feb 28, 2024

546d13274d...b3efb48673: remove now unnecessary explicit conversion to std::string in SimulationTest().

@vasild
Copy link
Contributor Author

vasild commented Mar 8, 2024

@maflcko, this has a NACK from you and I have addressed your concern. If not ACK, it would be nice if you can at least remove your NACK. Thanks for the review so far!

@vasild
Copy link
Contributor Author

vasild commented May 2, 2024

ping @maflcko

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Empact Empact Empact left review comments

@maflcko maflcko maflcko left review comments

@willcl-ark willcl-ark willcl-ark approved these changes

Assignees
No one assigned
Labels
P2P
Projects
High-priority for review
Status: Blockers
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
6 participants
@vasild @DrahtBot @epiccurious @Empact @maflcko @willcl-ark