<div class="gmail_quote">On Wed, Feb 22, 2012 at 4:15 AM, Jay Foad <span dir="ltr"><<a href="mailto:jay.foad@gmail.com">jay.foad@gmail.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
What do you think of the attached patch? It:<br>
<br>
1. Fixes the theoretical problem with GeneralHash::addUnaligned<br>
returning varying results on big-endian hosts.<br>
2. Fixes the strict aliasing problems that foiled my attempt to apply<br>
Meador's patch to improve uniquing of struct and function types.<br></blockquote><div><br></div><div>This is definitely the way to go. memcpy is the only safe technique here, and compilers should optimize it harder.</div>
<div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
3. Moves GeneralHash::finish and GeneralHash::mix out of line. This is<br>
just a matter of taste, but the inlined code looked a bit long-winded<br>
to me.<br></blockquote><div><br></div><div>Please leave these inline. They seem quite good candidates for inlining. Feel free to define them out of the class body, but still in the header file...</div><div><br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
It also simplifies and combines addAligned and addUnaligned into a<br>
single inline function addBytes that covers all cases, and relies on<br>
the compiler to optimise away the calls to memcpy. This mostly seems<br>
to work, except that GCC doesn't seem to be clever enough to optimise<br>
away the whole of:<br>
<br>
if (I != E) {<br>
Data = 0;<br>
std::memcpy(&Data, I, E - I);<br>
mix(Data);<br>
}<br>
<br>
... when addBits is called with a type which is known to be at least </blockquote><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">4-aligned. Can I change the code a bit to help the compiler out?<br>
</blockquote><div><br></div><div>No, the compiler should handle this. I completely agree with the decision to let the compile deduce and figure out alignment optimizations.</div><div><br></div><div>A few comments on the patch:</div>
<div><br></div><div><div>- template<typename T, bool isAligned></div><div>- struct addBitsImpl {</div><div>- static void add(GeneralHash &Hash, const T *I, const T *E) {</div><div>- Hash.addUnaligned(</div>
<div>- reinterpret_cast<const uint8_t *>(I),</div><div>- reinterpret_cast<const uint8_t *>(E));</div><div>+ // Add a range of bytes from I to E.</div><div>+ void addBytes(const char *I, const char *E) {</div>
<div>+ uint32_t Data;</div><div>+ // Use memcpy to work around strict aliasing rules.</div><div><br></div><div>Not work around, this is simply required by the standard... I don't think you need this comment at all actually.</div>
<div><br></div><div>+ for (; E - I >= ptrdiff_t(sizeof Data); I += sizeof Data) {</div><div>+ // A clever compiler should be able to turn this memcpy into a single</div><div>+ // aligned or unaligned load (depending on the alignment of the type T</div>
<div>+ // that was used in the call to addBits).</div><div>+ std::memcpy(&Data, I, sizeof Data);</div><div>+ mix(Data);</div><div> }</div><div>- };</div><div>-</div><div>- template<typename T></div>
<div>- struct addBitsImpl<T, true> {</div><div>- static void add(GeneralHash &Hash, const T *I, const T *E) {</div><div>- Hash.addAligned(</div><div>- reinterpret_cast<const uint32_t *>(I),</div>
<div>- reinterpret_cast<const uint32_t *>(E));</div><div>+ if (I != E) {</div><div>+ Data = 0;</div><div>+ std::memcpy(&Data, I, E - I);</div><div>+ mix(Data);</div><div> }</div><div>
- };</div><div>+ }</div><div> </div><div> // Add a range of bits from I to E.</div><div> template<typename T></div><div> void addBits(const T *I, const T *E) {</div><div>- addBitsImpl<T, AlignOf<T>::Alignment_GreaterEqual_4Bytes>::add(*this, I, E);</div>
<div>+ addBytes(reinterpret_cast<const char *>(I),</div><div>+ reinterpret_cast<const char *>(E));</div><div><br></div><div>Can we get rid of these reinterpret casts and retain the exact type all the way through? I don't see any real benefit to changing the types, and I think it'll make it more obvious to the reader.</div>
<div><br></div><div>It may also fix some of the alignment issues by forcing an instantiation with a particular type pointer for which the compiler knows more about the alignment.</div></div></div>