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

I1132 #362

Open
wants to merge 5 commits into
base: main
Choose a base branch
from
Open

I1132 #362

wants to merge 5 commits into from

Conversation

knizhnik
Copy link
Contributor

No description provided.

type ModifierTypeId = ModifierTypeId.Type

type VersionTag = VersionTag.Type
type VersionTag = util.ModifierId
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this is confusing reuse, which complicates things.
So I suggest to declare
object VersionTag extends TaggedType[util.ModifierId]

So we don't accidentally confuse two types.

In particular, is it always 32 long hash?
Also ScalaDoc is missing, so these assumptions should be described in ScalaDoc.

Copy link
Contributor Author

@knizhnik knizhnik Jul 8, 2020

Choose a reason for hiding this comment

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

I think that the main source of confusion is that there are many different types which are in0turn just wrappers around Array[Byte]:
ModifierId, VersionTag, EncodedTokenId, EncodedBoxId, ByteArrayWrapper
From my point of view I leave just one implementation (my choice is ByteArrayWrapper, because it most clear term) and may be left other types as aliases.
Concerning size of byte array: is it always 32 bytes or can have arbitrary length, I also do not know whether all hashes (keys) used in Ergo are 32 bytes long, but definitely it is not true for generic ByteArrayWrapper. Also my choice is to not make this assumption in ByteArrayWrapper implementation. If you think that speed of hash function is so critical, we can add just one extra if:

    override def hashCode: Int = {
      val bytes = hashBytes
      if (bytes.size == 32) {
         hashFromBytes(bytes(28), bytes(29), bytes(30), bytes(31))
      } else {
         java.util.Arrays.hashCode(bytes)
      }
    }

Base automatically changed from master to main February 3, 2021 23:09
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.

3 participants