Map out CNamePool and several related types #177
Conversation
Still missing one requested change, relating to the flexible array in `CNamePoolNodeInner`
|
Fixed most of what was requested, just awaiting guidance on the flexible array. Sorry about the formatting, my hook was messed up somehow, should be fixed now. |
Still missing one requested change, relating to the flexible array in `CNamePoolNodeInner`
Of note: - The flexible array `CNamePoolNodeInner::str` is now represented by a placeholder `char[1]` instead of an unsized array and can be accessed via `CNamePoolNodeInner::GetString()` - Both `CNamePoolNode` and `CNamePoolNodeInner` are now packed to an alignment of 4, matching their behavior in the game
include/RED4ext/CNamePool-inl.hpp
Outdated
| { | ||
| static UniversalRelocFunc<const char* (*)(const CName&)> func(Detail::AddressHashes::CNamePool_Get); | ||
| static UniversalRelocFunc<const char* (*)(const CName&)> func(Detail::AddressHashes::CNamePool_GetString); |
There was a problem hiding this comment.
Should be static const as this is never changing, same for the other relocs
include/RED4ext/CNamePool.hpp
Outdated
| /// @brief The corresponding string for #cname | ||
| /// This is a flexible array in the game, but because flexible arrays are a nonstandard extension, we use this | ||
| /// placeholder field plus #GetString() instead | ||
| const char str[1]; // 14 |
There was a problem hiding this comment.
Unsure what warning this produced that got mentioned in #177 (comment) but you should be able tk disable specific clang warnings per file or line with a comment, i find this hiding the meaning and just overall confusing. I would be for reverting this to older version where it was simply [] and adding comment to disable the failing clang check.
There was a problem hiding this comment.
That was a compiler error saying that this is not in the standard. I wouldn't like to disable a warning for this, even a line one, because if I'm correct both forms are not even standard behavior even in C (where this comes from).
At least this form doesn't throw a warning, even if it is undefined behavior. But if you guys decide to revert it, that's fine by me, I proposed to use this version because I'm more used it.
include/RED4ext/CNamePool-inl.hpp
Outdated
|
|
||
| RED4EXT_INLINE const char* RED4ext::CNamePoolNodeInner::GetString() const | ||
| { | ||
| return &*str; |
There was a problem hiding this comment.
Unsure why you did this exactly as returning just str should work even with the workaround you did.
include/RED4ext/CNamePool-inl.hpp
Outdated
|
|
||
| RED4EXT_INLINE RED4ext::CNamePoolNode* RED4ext::CNamePoolNode::NextInList() const | ||
| { | ||
| return reinterpret_cast<CNamePoolNode*>(reinterpret_cast<uintptr_t>(&inner) + len); |
There was a problem hiding this comment.
Use std::bitcast instead of reinterpret casts
include/RED4ext/CNamePool-inl.hpp
Outdated
|
|
||
| RED4EXT_INLINE RED4ext::CNamePoolNodeInner* RED4ext::CNamePoolNodeInner::NextInList() const | ||
| { | ||
| return &const_cast<CNamePoolNodeInner*>(this)->Outer()->NextInList()->inner; |
There was a problem hiding this comment.
Please, dont do this, just make a const function variants. You may use const_cast there to call non-const version but this is not the best way in my opinion atm a there are no real const variants.
You also probably dont want to return pointers to mutable data in const functions, they are const because data inside shouldnt be changing.
There was a problem hiding this comment.
Actually on that note, Im not really sold now on Outer being non-const and having mutable version for other than private internal use.
There was a problem hiding this comment.
A lot of functions could use const versions from reading the comments you wrote, with mutable versions being privated and probably marked as friends for classes that truly need them writable.
It seems very error prone not returning const pointers for nodes from what I read overall, game should be managing them and not end user in any way. Feels like API flaw.
include/RED4ext/CNamePool.hpp
Outdated
| const CName cname; // 00 | ||
|
|
||
| /// @brief The next node in the hash bucket containing this node | ||
| CNamePoolNodeInner* next; // 08 |
There was a problem hiding this comment.
From all I read and saw, this should probably be mutable const and not just regular pointer.
include/RED4ext/CNamePool.hpp
Outdated
| struct CNamePoolNode | ||
| { | ||
| // Only the game should allocate this struct | ||
| CNamePoolNode() = delete; |
There was a problem hiding this comment.
Also should probably delete destructors for both no?
include/RED4ext/CNamePool.hpp
Outdated
| /// @brief The size of #inner in bytes | ||
| const uint32_t len; // 00 | ||
| /// @brief The actual allocated space | ||
| CNamePoolNodeInner inner; // 04 |
There was a problem hiding this comment.
Also should probably be mutable const instead
There was a problem hiding this comment.
I looked at the rest and a lot could/should be const or mutable const at least imo, ill stop commenting them to not pollute the review... Think you get the overall idea from the rest, we can discuss in detail on Discord I suppose.
There was a problem hiding this comment.
And after looking at the iterators, also considering now if it wouldnt be better to just hide them inside CNamePool as private structures. Maybe then the dangers I am concerned about would be partially gone.
| */ | ||
| struct CNamePoolAllocator | ||
| { | ||
| class Iterator |
There was a problem hiding this comment.
Unsure if both of the iterators shouldnt also be const in nature, these feel errorprone. Maybe something in the collections requires some write access but that should probably be fixed or there could be something more proper done for that case. Dont think there should be mutable iterators for public use though like there are now.
Maybe would consider hiding these inside a relevant class at least as private structures.
- CNamePoolNode is now private and moved to CNamePoolAllocator::CNamePoolNodeAllocWrapper - CNamePoolNodeInner is now CNamePoolNode - The API should hopefully be as const as possible
Fills out CNamePool and all its internal types (that I could figure out). There's still a bit left un-reversed but I think this covers anything you'd want to do that couldn't already be done with the existing static methods. I went ahead and tried to add Doxygen-style docs to pretty much everything cause who doesn't love documentation?