Fix strict aliasing violations in database_utils.hpp #131
Conversation
| std::string s = "0x"; | ||
| s.append( to_hex( reinterpret_cast<const char*>(&as_bytes), sizeof(as_bytes) ) ); |
There was a problem hiding this comment.
Cast to const char* is legal and doesn't require memcpy. This could just drop as_bytes and cast &f.
There was a problem hiding this comment.
Are you suggesting to change it? I'm mostly indifferent, as it was already approved previously like this.
There was a problem hiding this comment.
Up to you. It's not a big deal, and the code works either way.
| @@ -106,35 +106,61 @@ namespace fc { | |||
| from_variant(v, oid._id); | |||
| } | |||
|
|
|||
| inline | |||
| void float64_to_double (const float64_t& f, double& d) { | |||
There was a problem hiding this comment.
softfloat.hpp has has double from_softfloat64(float64_t) and float64_t to_softfloat64(double). I would use those and define the float128 versions similarly. (Note that C++20 has std::bit_cast)
There was a problem hiding this comment.
At first I wasn't sure if eos-vm considered these softfloat functions part of its public interface. But it seems they're present even without ENABLE_SOFTFLOAT so maybe it does. The code reuse would probably be a good thing, I suppose, even in this trivial case.
There was a problem hiding this comment.
They're not from eos-vm. They're from berkeley softfloat.
There was a problem hiding this comment.
Well that's the thing, we bring in softfloat two places: eos-vm and as a library in eosio.
eos-vm exposes the softfloat headers in a way that suggests they're public interface. (it's included in a cmake PUBLIC interface -- which they probably need to be due to being a header only library -- but they're not in an /impl/ or /detail/ directory and/or namespace either)
The library that's pulled for the rest of the eosio build is primarily used for softfloat injection in EOS VM OC. But mid/long term I expect that injection to go away. There are a few very very minor usages of softfloat outside of EOS VM OC injection -- funny enough already in database_utils.hpp does it include softfloat.hpp. Kinda lousy to double down depending on this library for just a few lines of code here and there; even more lousy if we need to go add float128 versions to said library 😕
There was a problem hiding this comment.
We already have a direct dependency here in the types float32_t and float64_t
There was a problem hiding this comment.
If we do eliminate those types, then the dependency on softfloat sill naturally go away. Anyway, I'm approving this PR as-is. Feel free to take or ignore my suggestions at your own discretion.
There was a problem hiding this comment.
I'll go ahead and take as-is since 1) it was approved in the past and 2) there doesn't seem to be anything straight out incorrect with the changes. You can submit a PR with your improvements if you have a chance.
This ports over EOSIO/eos#9761