jovibor / HexCtrl

Fully-featured Hex Control written in C++/MFC.
Other
164 stars 61 forks source link

Data interpreter refinements #8

Closed datasynergyuk closed 4 years ago

datasynergyuk commented 4 years ago

Minor fixes for data interpreter. Mostly self-explanatory.

Some are issues that were lost in recent translation from PR and others are issues that were present in original PR. FILETIME, OLE time, time32_t, time64_t, Javatime all checked against third-party tool and work ok. Exception is FILETIME BE which appears to not work correctly in third-party tool.

Please note use to memcpy() to workaround apparent problem converting from QWORD to Double for OLE time. Static_cast<> doesn't convert this correctly but I'm not sure why. NB: OLE time is a Double. A value of 2.0 indicates 1st January 1900.

jovibor commented 4 years ago

Please note use to memcpy() to workaround apparent problem converting from QWORD to Double for OLE time. Static_cast<> doesn't convert this correctly but I'm not sure why.

As just the opposite, static_cast converts exactly right. But what you do want exactly? Convert or copy bits' layout?

If the latter, then static_cast was never meant to do this kind of things. It's simply to convert between data types.

To step through C++ type aliasing barriers you have to use whether std::memcpy or one old known trick:

QWORD qword = 123ULL;
double dbl = *reinterpret_cast<double*>(&qword);

In C++20 though there is a built-in std::bit_cast machinery for that.

datasynergyuk commented 4 years ago

Ok. I didn't know the *reinterpret_cast<> trick. It still seems a relatively unsafe / non-portable thing to do but certainly better than blindly copying bits with memcpy(). Thank you.

jovibor commented 4 years ago

Ok. I didn't know the *reinterpret_cast<> trick. It still seems a relatively unsafe / non-portable thing to do but certainly better than blindly copying bits with memcpy(). Thank you.

Incorrect.

It's called "trick" for reason. Yes it works almost always... From the official documentation i gave the link above:

Notes reinterpret_cast (or equivalent explicit cast) between pointer or reference types shall not be used to reinterpret object representation in most cases because of the type aliasing rule. Before std::bit_cast, std::memcpy can be used when it is needed to interpret the object representation as one of another type:

template <class To, class From>
typename std::enable_if<
(sizeof(To) == sizeof(From)) &&
std::is_trivially_copyable<From>::value &&
std::is_trivial<To>::value,
// this implementation requires that To is trivially default constructible
To>::type
// constexpr support needs compiler magic
bit_cast(const From &src) noexcept
{
To dst;
std::memcpy(&dst, &src, sizeof(To));
return dst;
}

std::bit_cast is based on std::memcpy.

datasynergyuk commented 4 years ago

Thank you for adopting most of my suggestions into HexCtrl.

Please can I ask why *reinterpret_cast changed back to memcpy() for the OLE date? I understood you had previously told me this was the wrong approach. I am sorry if I misunderstood

Please can I suggest:

a. Replace "N/A" with a constant (for string de-duplication)

b. Please consider implementation of wstr2num(). This will now interpret strings that start with 0 as octal e.g. A user could input 0123 but it would be interepreted as 0x53. Is this deliberate?

jovibor commented 4 years ago

Please can I ask why *reinterpret_cast changed back to memcpy() for the OLE date?

They have the same effect. I explained it all above.

Please consider implementation of wstr2num(). This will now interpret strings that start with 0 as octal e.g. A user could input 0123 but it would be interepreted as 0x53. Is this deliberate?

0 is a standard octal prefix. This is by design.

datasynergyuk commented 4 years ago

Majority of suggestions now incorporated in HexCtrl