[llvm-commits] [llvm] r150890 - in /llvm/trunk: include/llvm/ADT/Hashing.h lib/Support/CMakeLists.txt lib/Support/Hashing.cpp unittests/ADT/HashingTest.cpp unittests/CMakeLists.txt
Talin
viridia at gmail.com
Mon Feb 20 22:43:03 PST 2012
On Mon, Feb 20, 2012 at 10:38 AM, Jay Foad <jay.foad at gmail.com> wrote:
> On 18 February 2012 21:00, Talin <viridia at gmail.com> wrote:
> > Added: llvm/trunk/include/llvm/ADT/Hashing.h
>
> Nitpick: wouldn't this be better in ../Support ?
>
I'm not sure what the boundary between ADT and Support is - I put it in ADT
because containers use it. However, if someone moves it to Support I won't
complain.
>
> > + /// Add a float
> > + GeneralHash& add(float Data) {
> > + union {
> > + float D; uint32_t I;
> > + };
> > + D = Data;
> > + addInt(I);
> > + return *this;
> > + }
> > +
> > + /// Add a double
> > + GeneralHash& add(double Data) {
> > + union {
> > + double D; uint64_t I;
> > + };
> > + D = Data;
> > + addInt(I);
> > + return *this;
> > + }
>
> IMO it would be better not to implement these at all until someone
> needs them, and decides what to do about the +/- 0 problem. (But
> that's just another nitpick!)
>
> For uniquing of ConstantFPs, I think you do want to distinguish between
+/- 0.
> > +// Add a possibly unaligned sequence of bytes.
> > +void GeneralHash::addUnaligned(const uint8_t *I, const uint8_t *E) {
> > + ptrdiff_t Length = E - I;
> > + if (uintptr_t(I) & 3 == 0) {
> > + while (Length > 3) {
> > + mix(*reinterpret_cast<const uint32_t *>(I));
> > + I += 4;
> > + Length -= 4;
> > + }
> > + } else {
> > + while (Length > 3) {
> > + mix(
> > + uint32_t(I[0]) +
> > + (uint32_t(I[1]) << 8) +
> > + (uint32_t(I[2]) << 16) +
> > + (uint32_t(I[3]) << 24));
> > + I += 4;
> > + Length -= 4;
> > + }
> > + }
>
> I think there's a serious problem here on big-endian hosts, because
> identical arrays of bytes will hash to different values depending on
> whether they happen to start on a 4-byte boundary or not.
>
Good catch. Shouldn't be hard to fix I would think.
>
> Thanks for working on this!
> Jay.
>
--
-- Talin
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20120220/5aafe0f2/attachment.html>
More information about the llvm-commits
mailing list