Fix c++20 compilation problem for clang10 #1228
Conversation
include/json/allocator.h
Outdated
| void deallocate(volatile pointer p, size_type n) { | ||
| std::memset(p, 0, n * sizeof(T)); | ||
| void deallocate(pointer p, size_type n) { | ||
| volatile pointer do_not_optimize_memset_p = p; |
There was a problem hiding this comment.
I really don't see how this ever prevented an optimizer from omitting the memset call.
Again this is just a volatile pointer to T, NOT a pointer to volatile T.
Pointer values are not modified by a memset call, so from the optimizer's PoV, it doesn't matter if the pointer changes or not. A local variable or function parameter (equivalently) that is not leaked outside the function has a provable scope of visible effects that the optimizer can exploit whether it's marked volatile or not. My opinion about this variable is that the volatile applied only to the parameter local and didn't affect the function signature before. Same as a const parameter is ignored. It can be removed without an ABI break. Further, the do_not_optimize_memset_p has no effect and we might as well just memset p directly.
There was a problem hiding this comment.
What you write sounds reasonable to me. However, the codes intention is stated clearly in the comment. Hence, the question: is this actually a bug?
There was a problem hiding this comment.
I think this may have been a "faith-based" use of volatile. It happens. It should be replaced with something that actually works.
I think we want the C11 memset_s on platforms on which it's available.
It is guaranteed not to be optimized away, and memset has no such guarantee, regardless of any irrelevant volatile qualifications on the pointer argument.
There was a problem hiding this comment.
I updated the PR with memset_s. Indeed according to the c++ doc this should never get optimized away.
I hope my usage is correct.
Thanks for your comments.
ledergec
force-pushed
the
fix_compilation_problem_with_c++20_standard
branch
from
3279095 to
6e442c5
Compare
…o compiler optimization
ledergec
force-pushed
the
fix_compilation_problem_with_c++20_standard
branch
from
6e442c5 to
8a923cb
Compare
|
I feel like this is ready to be merged? Anything blocking? |
|
Looks good to me. |
This fixes the following issue:
#1193