Alternative to rtti for the event bus. #1942
Conversation
WalkthroughThe event type hashing mechanism in the EventBus system is refactored to support both RTTI-enabled and RTTI-disabled environments. A macro abstracts hash retrieval, and type definitions are updated to use a new Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant EventBus
participant Listener
Client->>EventBus: listen(PCSX_HASH_TYPE(Event), Listener)
EventBus->>EventBus: Register listener with hash key
Client->>EventBus: signal(PCSX_HASH_TYPE(Event), event)
EventBus->>Listener: Notify listener(s) for hash key
Poem
✨ Finishing Touches
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
There was a problem hiding this comment.
Pull Request Overview
This PR introduces an alternative mechanism to retrieve event type hashes for the event bus without relying solely on RTTI.
- Implements conditional inclusion to use standard RTTI when available and a fallback (ctti) when not.
- Updates hashtable key types and listener registrations to use the PCSX_HASH_TYPE macro.
Comments suppressed due to low confidence (2)
src/support/eventbus.h:41
- Consider adding a brief comment above the PCSX_HASH_TYPE macro to describe its purpose and how it differentiates between using RTTI and the ctti library, for improved clarity and maintainability.
#define PCSX_HASH_TYPE(type) typeid(type).hash_code()
src/support/eventbus.h:55
- A comment clarifying why HashType is typedef’d differently based on the availability of RTTI (i.e., std::size_t vs. std::uint64_t) would be helpful to future maintainers.
typedef std::size_t HashType;
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/support/eventbus.h(5 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (13)
- GitHub Check: pcsx-redux (x86_64-linux)
- GitHub Check: pcsx-redux (aarch64-linux)
- GitHub Check: Codacy Static Code Analysis
- GitHub Check: toolchain
- GitHub Check: coverage
- GitHub Check: aur-build
- GitHub Check: asan
- GitHub Check: build-openbios
- GitHub Check: build
- GitHub Check: cross-arm64
- GitHub Check: macos-build-and-test-toolchain
- GitHub Check: pcsx-redux (x86_64-linux)
- GitHub Check: pcsx-redux (aarch64-linux)
🔇 Additional comments (3)
src/support/eventbus.h (3)
37-48: RTTI detection and macro definition looks solid.The conditional compilation logic correctly detects RTTI support using standard compiler macros and provides appropriate fallbacks. The macro abstraction cleanly handles the different hashing mechanisms.
54-58: HashType abstraction is well-designed.The conditional typedef correctly matches the return types of the respective hashing mechanisms -
std::size_tfor RTTI'stypeid().hash_code()andstd::uint64_tfor CTTI's hash method.
90-90: Hash table key type update is consistent.The typedef correctly uses the new
HashTypeinstead of the hardcodedstd::size_t, maintaining consistency with the abstraction.
| void signal(const Event& event) { | ||
| using Functor = typename ListenerElement<Event>::Functor; | ||
| auto list = m_table.find(typeid(Event).hash_code()); | ||
| auto list = m_table.find(PCSX_HASH_TYPE(Event)); |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Consider potential ABI compatibility implications.
The macro usage is correct and maintains a clean interface. However, switching between RTTI and non-RTTI modes could break ABI compatibility since the hash types and algorithms differ.
Consider documenting the ABI implications of this change and ensure that:
- All translation units use the same RTTI mode
- The build system enforces consistent RTTI settings
- Different hash algorithms don't introduce subtle behavioral differences
Also applies to: 112-112, 127-127
🏁 Script executed:
#!/bin/bash
# Description: Check for potential mixed RTTI usage patterns in the codebase
# Look for direct typeid usage that might bypass the macro
rg "typeid\(" --type cpp --type cc --type cxx | grep -v "PCSX_HASH_TYPE"
# Check for existing RTTI-dependent code that might conflict
rg -i "rtti" --type cpp --type cc --type cxx --type cmake | head -10
# Look for build configuration that might affect RTTI settings
rg -i "frtti\|fno-rtti" --type cmake --type makeLength of output: 246
The previous ripgrep invocation failed to detect “.cc” files, so let’s retry with explicit glob patterns:
🏁 Script executed:
#!/bin/bash
# 1) Find any direct typeid() usages not wrapped in PCSX_HASH_TYPE
rg "typeid\\(" -g '*.cpp' -g '*.cc' -g '*.cxx' --no-heading | grep -v "PCSX_HASH_TYPE"
# 2) Scan for any RTTI mentions in source, headers, and CMake
rg -i rtti -g '*.cpp' -g '*.cc' -g '*.cxx' -g '*.h' -g '*.hpp' -g '*.cmake' -g 'CMakeLists.txt' | head -20
# 3) Look for RTTI-related compiler flags in CMakeLists
rg -i 'fno-rtti|frtti' -g '*.cmake' -g 'CMakeLists.txt' --no-headingLength of output: 2188
Document ABI implications and enforce RTTI consistency
No direct typeid() calls bypassing PCSX_HASH_TYPE were found in your sources (only in third_party/EABase tests). However, the build system doesn’t currently lock down RTTI settings, which risks mixing RTTI modes and breaking ABI. To address this:
- In
src/support/eventbus.h, expand the comment aroundPCSX_RTTIto explain that it switches hash algorithms and impacts the ABI. - In your top‐level CMakeLists.txt (and any per‐module CMake files), add compiler flags (
-frttior-fno-rtti) uniformly so all targets share the same RTTI mode. - Consider a CMake configuration check that errors out if different targets compile with mismatched RTTI settings.
🤖 Prompt for AI Agents
In src/support/eventbus.h at line 102, update the comment near the
PCSX_HASH_TYPE(Event) macro usage to explicitly document that switching between
RTTI and non-RTTI modes changes the hash algorithm and affects ABI
compatibility. Additionally, ensure the top-level CMakeLists.txt and any
module-specific CMake files enforce consistent RTTI compiler flags (-frtti or
-fno-rtti) across all targets, and implement a CMake configuration check that
fails the build if RTTI settings differ between targets to prevent ABI breakage.
| #define CTTI_HAS_ENUM_AWARE_PRETTY_FUNCTION 1 | ||
| #define CTTI_HAS_VARIABLE_TEMPLATES 1 | ||
| #include "ctti/type_id.hpp" | ||
| #define PCSX_HASH_TYPE(type) ctti::unnamed_type_id<type>().hash() |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Verify CTTI library configuration and compatibility.
The CTTI library configuration defines are hardcoded. Ensure these settings are compatible with your target environments and the version of CTTI being used.
🏁 Script executed:
#!/bin/bash
# Description: Check if CTTI library is properly integrated and verify the hash return type
# Check if CTTI headers exist in the project
fd -t f -e hpp -e h "type_id" | head -10
# Look for CTTI-related build configuration
rg -i "ctti" --type cmake --type make | head -5
# Verify the hash method return type in CTTI documentation or source
rg -A 5 -B 5 "hash.*uint64" | head -10Length of output: 18316
CTTI Library Integration and Compatibility Needs Attention
- No
ctti/type_id.hpp(or other CTTI headers) were found in the repository; confirm the CTTI library is added as a dependency (e.g. submodule or external package) and that your build system (CMake/Makefiles) includes its header directory. - There are no CTTI-related entries in CMakeLists.txt or Makefiles—add the necessary
find_packageor include-path settings so the compiler can locate the headers. - The hardcoded macros
must match the actual CTTI version you’re using. Verify your version supports these features before defining them.
#define CTTI_HAS_ENUM_AWARE_PRETTY_FUNCTION 1 #define CTTI_HAS_VARIABLE_TEMPLATES 1
- Ensure that
ctti::unnamed_type_id<type>().hash()returns the intended width (e.g.,std::size_toruint64_t) on all target platforms. If you depend on a 64-bit hash, guard or static-assert accordingly.
🤖 Prompt for AI Agents
In src/support/eventbus.h around lines 44 to 47, verify that the CTTI library is
properly integrated by ensuring its headers are present in the project and that
your build system (CMake or Makefiles) includes the correct include paths or
find_package directives for CTTI. Confirm the hardcoded macros
CTTI_HAS_ENUM_AWARE_PRETTY_FUNCTION and CTTI_HAS_VARIABLE_TEMPLATES match the
actual CTTI version used, adjusting or removing them if incompatible.
Additionally, check that ctti::unnamed_type_id<type>().hash() returns the
expected hash width on all target platforms, adding static assertions or guards
if a specific size like 64-bit is required.
1 participant
No description provided.