Skip to content

try 2: allow hash() to take multiple arguments similar to pack() #1719

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Draft
wants to merge 6 commits into
base: main
Choose a base branch
from

Conversation

spoonincode
Copy link
Member

This is another attempt to allow passing hash() multiple arguments to concatenate together similar to how pack() already does. This can allow some more concise usage of hash() without resorting to make_tuple/pair() and remembering to avoid the trap of needing to use cref along with make_tuple/pair() to avoid copies.

Based on this comment chain #1526 (comment) I have modified the hash impls such that,

  • hash_raw() takes a ContiguousCharSource that it hashes directly -- this is the previous behavior when using these nontemplated call forms:
    static sha1 hash( const char* d, uint32_t dlen );
    static sha1 hash( const std::string& );
  • hash() now is variadic and hashes with the same rules of our serialization format

Of course this means that usage of hash(std::string()) now has different output between these two revisions so any usages of that which matter (many don't!) need to be migrated to hash_raw(std::string()).

So the way I approached this was to first convert all calls to the two forms above to hash_raw(), and then go back and make a determination for sites that are fine to transition to simple hash() in order to limit the number of hash_raw() calls. But that was a huge pain: so this PR just leaves all calls to the two forms above as hash_raw() but that means the change set is large.

Ultimately I'm not really all that confident in making such a huge change.

@spoonincode
Copy link
Member Author

oof.. foiled by usage in contract tests

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

Successfully merging this pull request may close these issues.

2 participants