Skip to content
This repository was archived by the owner on Oct 12, 2022. It is now read-only.

Conversation

denis-sh
Copy link
Contributor

@denis-sh denis-sh commented Mar 6, 2014

@denis-sh
Copy link
Contributor Author

denis-sh commented Mar 6, 2014

As always hacking with our poor try to implement collections support in compiler and runtime fixing one issue discovers two (Issue 12303 & Issue 12304).

About test failure: it doesn't look like a my fault.

|| cast(const TypeInfo_AssociativeArray) element
|| cast(const ClassInfo) element
|| cast(const TypeInfo_Interface) element;
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Downcasts are expensive and explicitly listing all TypeInfos that define getHash might easily break.
Even though it's slow too, I'd suggest to use TypeInfo.getHash for each element.

@denis-sh
Copy link
Contributor Author

denis-sh commented Mar 7, 2014

Downcasts are expensive and explicitly listing all TypeInfos that define getHash might easily break.
Even though it's slow too, I'd suggest to use TypeInfo.getHash for each element.

Changed to use getHash in case of small (count <= 8) arrays to not waste performance. What about to add hasCustomToHash property to TypeInfo? This will solve both performance and maintaining issues.

You can't use + or ^ here because the order of elements wouldn't affect the hash.

Yes I can. The fact it's not perfect doesn't mean I can't do it. Also it is already done in runtime and changing of hashing algorithm is a subject of a separate pull request.

Sometimes like in sometimes?

Nop. It's Issue 12303 & Issue 12304, the usage is quasi-random. Comment improved.

Added private and also added @safe for the last unittest.

@MartinNowak
Copy link
Member

You can't use + or ^ here because the order of elements wouldn't affect the hash.

Yes I can. The fact it's not perfect doesn't mean I can't do it. Also it is already done in runtime and changing of hashing algorithm is a subject of a separate pull request.

Fine, we still need to address these issues.

What about to add hasCustomToHash property to TypeInfo?

I don't want to add more stuff to TypeInfo currently. When we succeed with the library AA all those cruft can go.

@denis-sh
Copy link
Contributor Author

denis-sh commented Mar 8, 2014

This compromise is worse than the original code, because it uses different hashing methods depending on the number of array elements.

I changed it back as I feel the same, but also I don't see a real reason. What is the problem with choosing a hash algorithm based on number of elements except some bad feeling about it?

@MartinNowak
Copy link
Member

First you never want to do surprising things in an implementation, e.g. because it makes it harder to track down bugs. Say that hasCustomToHash becomes incorrect you'd get mad reducing a bug which is only triggered for > 8 elements.

MartinNowak added a commit that referenced this pull request Mar 8, 2014
@MartinNowak MartinNowak merged commit dc3e69a into dlang:master Mar 8, 2014
@MartinNowak
Copy link
Member

Thanks

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants