Bug 115008
Summary: | Compute "better" hash for WTF::StringImpl::StringImpl(CreateEmptyUnique_T) | ||
---|---|---|---|
Product: | WebKit | Reporter: | David Kilzer (:ddkilzer) <ddkilzer> |
Component: | Web Template Framework | Assignee: | Nobody <webkit-unassigned> |
Status: | NEW | ||
Severity: | Normal | CC: | benjamin, darin |
Priority: | P2 | ||
Version: | 528+ (Nightly build) | ||
Hardware: | Unspecified | ||
OS: | Unspecified |
David Kilzer (:ddkilzer)
Per comments in Bug 114970, we should compute a "better" hash in WTF::StringImpl::StringImpl(CreateEmptyUnique_T):
(In reply to Bug 114970 comment #2, Darin Adler wrote)
> (From update of attachment 199053 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=199053&action=review
>
> > Source/WTF/wtf/text/StringImpl.h:316
> > - unsigned hash = reinterpret_cast<uintptr_t>(this);
> > + unsigned hash = static_cast<uint32_t>(reinterpret_cast<uintptr_t>(this));
>
> This is a silly approach to creating the hash. For example, the low bits will always be due to alignment zero. I suggest we use a random number instead.
>
> Code change is OK, but code is dumb.
(In reply to Bug 114970 comment #6, David Kilzer wrote)
> (In reply to Bug 114970 comment #2, Darin Adler wrote)
> > Code change is OK, but code is dumb.
>
> Should we use WTF::intHash(uint32_t) on 32-bit platforms and WTF::intHas(uint64_t) on 64-bit platforms instead?
(In reply to Bug 114970 comment #7, Benjamin Poulain wrote)
> (In reply to Bug 114970 comment #6, David Kilzer wrote)
> > Should we use WTF::intHash(uint32_t) on 32-bit platforms and WTF::intHas(uint64_t) on 64-bit platforms instead?
>
> The only property needed for that hash is it should never equals the hash of emptyString().
>
> We could basically use a constant here. Any number !m hash(empty()) has the same theoretical chances of conflicts.
>
> I have been trying to kill this constructor every now and then :)
(In reply to Bug 114970 comment #8, Darin Adler wrote)
> (In reply to Bug 114970 comment #7, Benjamin Poulain wrote)
> > The only property needed for that hash is it should never equals the hash of emptyString().
>
> Maybe (hash(emptyString()) ^ 0x100) would work?
Attachments | ||
---|---|---|
Add attachment proposed patch, testcase, etc. |