util: Use void* throughout support/lockedpool.h #16195
Conversation
promag
left a comment
promag
left a comment
There was a problem hiding this comment.
ACK 018c7d3, code change looks good, just 2 comments for your consideration.
|
@jkczyz Nice first-time contribution! Welcome as a contributor! What compiler is it that is failing to compile? |
|
@practicalswift Thanks! My organization uses a customized STL. It provides a char* specialization for std::hash, which contains a static_assert that always fails. This is to prevent code from keying by pointers when keying by the pointed-to string is what is probably desired. |
|
@jkczyz Thanks for the clarification! Please squash into one commit. Please report any other issues or warnings you encounter when using your organisations' compiler environment. Diversity in testing is good :-) |
|
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. ReviewsSee the guideline for information on the review process.
If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update. ConflictsNo conflicts as of last run. |
jkczyz
force-pushed
the
2019-06-arena-key
branch
from
f954a1b to
102b565
Compare
|
Concept ACK @sipa can you please comment on this? you seem to have a good grasp around subtle issues with regard to UB around pointer casts |
|
My understanding is that casting something to void* and then back to the correct pointer type before use is always fine. Will review the code soon. |
jkczyz
force-pushed
the
2019-06-arena-key
branch
from
102b565 to
83dbd78
Compare
Replace uses of char* with void* in Arena's member variables. Instead, cast to char* where needed in the implementation. Certain compiler environments disallow std::hash<char*> specializations to prevent hashing the pointer's value instead of the string contents. Thus, compilation fails when std::unordered_map is keyed by char*. Explicitly using void* is a workaround in such environments. For consistency, void* is used throughout all member variables similarly to the public interface.
jkczyz
force-pushed
the
2019-06-arena-key
branch
from
83dbd78 to
f36d1d5
Compare
There was a problem hiding this comment.
Code-review ACK f36d1d5
The member variables of the Arena class are changed from char* to void* and casts in the code to char* are done whenever pointer arithmetic is involved (which wouldn't work with void*). LGTM.
My understanding is that casting something to void* and then back to the correct pointer type before use is always fine.
That would also be my understanding.
|
This PR has had a couple of acks for a while now and could be ready to merge. Is this change still relevant and valid? |
|
ACK f36d1d5 |
I'm not longer employed with the organization in questions, FWIW. This patch was required to compile in their environment as noted in #16195 (comment). |
f36d1d5 Use void* throughout support/lockedpool.h (Jeffrey Czyz) Pull request description: Replace uses of char* with void* in Arena's member variables. Instead, cast to char* where needed in the implementation. Certain compiler environments disallow std::hash<char*> specializations to prevent hashing the pointer's value instead of the string contents. Thus, compilation fails when std::unordered_map is keyed by char*. Explicitly using void* is a workaround in such environments. For consistency, void* is used throughout all member variables similarly to the public interface. Changes to this code are covered by src/test/allocator_tests.cpp. ACKs for top commit: achow101: ACK f36d1d5 theStack: Code-review ACK f36d1d5 jonatack: ACK f36d1d5 review, debug build, unit tests, checked clang 15 raises "error: arithmetic on a pointer to void" without the conversions here from the generic void* pointer back to char* Tree-SHA512: f9074e6d29ef78c795a512a6e00e9b591e2ff34165d09b73eae9eef25098c59e543c194346fcd4e83185a39c430d43744b6f7f9d1728a132843c67bd27ea5189
10 participants
Replace uses of char* with void* in Arena's member variables. Instead,
cast to char* where needed in the implementation.
Certain compiler environments disallow std::hash<char*> specializations
to prevent hashing the pointer's value instead of the string contents.
Thus, compilation fails when std::unordered_map is keyed by char*.
Explicitly using void* is a workaround in such environments. For
consistency, void* is used throughout all member variables similarly to
the public interface.
Changes to this code are covered by src/test/allocator_tests.cpp.