Networking: hash RPCs by compiler-independent type name#204
Conversation
typeid(T).name() is not portable across compilers (MSVC emits a readable decorated name, the Itanium ABI a mangled one), so an MSVC client and a GCC server computed different CRC32 hashes for the same RPC type and silently dropped each other's RPCs (chat, weather, SetTransform game RPC). Add Utils::TypeName<T>(), which normalizes __FUNCSIG__/__PRETTY_FUNCTION__ to an identical string across MSVC/GCC/Clang, plus a per-type RPC identity cache (RPCHash/RPCName) shared by IRPC and IGameRPC. Covered by unit tests in tests/modules/type_name_ut.h.
WalkthroughIntroduces ChangesCompiler-independent RPC Identity
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
code/tests/modules/type_name_ut.h (2)
25-27: ⚡ Quick winAdd a closing comment for the nested namespace.
Please close
namespace Innerwith an explicit comment to match the repository namespace-closing rule.Suggested patch
namespace Inner { struct Delta {}; - } + } // namespace InnerAs per coding guidelines, "Use namespaces with closing comment pattern: namespace Framework::SubModule { ... } // namespace Framework::SubModule".
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@code/tests/modules/type_name_ut.h` around lines 25 - 27, The nested namespace Inner is missing a closing comment on its closing brace. Add an explicit closing comment after the closing brace of namespace Inner to match the repository's namespace-closing rule. The comment should follow the pattern // namespace Inner to clearly indicate which namespace is being closed.Source: Coding guidelines
14-15: ⚡ Quick winInclude
<string>directly wherestd::stringis used.This test uses
std::string(Lines 62-63) but relies on transitive includes. Add a direct include to keep compile dependencies explicit.Suggested patch
`#include` <string_view> +#include <string>Also applies to: 62-63
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@code/tests/modules/type_name_ut.h` around lines 14 - 15, The file uses std::string in the test code but relies on transitive includes to provide it rather than directly including the header. Add a direct include for <string> in the include section near where <string_view> is included to make the compile dependency explicit and avoid relying on transitive includes from other headers.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@code/tests/modules/type_name_ut.h`:
- Around line 25-27: The nested namespace Inner is missing a closing comment on
its closing brace. Add an explicit closing comment after the closing brace of
namespace Inner to match the repository's namespace-closing rule. The comment
should follow the pattern // namespace Inner to clearly indicate which namespace
is being closed.
- Around line 14-15: The file uses std::string in the test code but relies on
transitive includes to provide it rather than directly including the header. Add
a direct include for <string> in the include section near where <string_view> is
included to make the compile dependency explicit and avoid relying on transitive
includes from other headers.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: b33b1407-1d94-413d-81c0-f503a11a9108
📒 Files selected for processing (6)
code/framework/src/networking/rpc/game_rpc.hcode/framework/src/networking/rpc/rpc.hcode/framework/src/networking/rpc/rpc_identity.hcode/framework/src/utils/type_name.hcode/tests/framework_ut.cppcode/tests/modules/type_name_ut.h
|
This seems to be addressed by #201 . I'll hold on to the |
1 participant
Came across this issue when attempting to chat from a client running
hogwarts-mpon Windows against a Ubuntu 24.04 Linux VM.typeid(T).name() is not portable across compilers (MSVC emits a readable decorated name, the Itanium ABI a mangled one), so an MSVC client and a GCC server computed different CRC32 hashes for the same RPC type and silently dropped each other's RPCs (chat, weather, SetTransform game RPC).
Add Utils::TypeName(), which normalizes FUNCSIG/PRETTY_FUNCTION to an identical string across MSVC/GCC/Clang, plus a per-type RPC identity cache (RPCHash/RPCName) shared by IRPC and IGameRPC. Covered by unit tests in tests/modules/type_name_ut.h.
Question: Does this require a version change?
Also verified to work with Windows client against Linux server.
GHA checks will still fail until #203 is merged.
Summary by CodeRabbit
Refactor
Tests