Skip to content

Conversation

@proberts-cinesite
Copy link
Contributor

Adds the ability to reconstruct a IECore.MurmurHash from its string representation, which is useful if you want to store the hash as StringData in a primitive variable for example.

Breaking Changes

None

Checklist

  • I have read the contribution guidelines.
  • I have updated the documentation, if applicable.
  • I have tested my change(s) in the test suite, and added new test cases where necessary.
  • My code follows the Cortex project's prevailing coding style and conventions.

Copy link
Member

@johnhaddon johnhaddon left a comment

Choose a reason for hiding this comment

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

Thanks Paul - this looks handy. Comments inline...

Comment on lines 89 to 90
std::string toString() const;
void fromString( const std::string &repr );
Copy link
Member

Choose a reason for hiding this comment

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

I really like the symmetry of these two functions - I think it makes it very clear that we're serialising to a string and deserialising back.

Comment on lines 64 to 65
// Construct directly from string representation
inline explicit MurmurHash( const std::string &repr );
Copy link
Member

Choose a reason for hiding this comment

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

Do we need this constructor as well as fromString()? I don't see the need to have two ways of achieving the same thing, and the constructor's semantics seem much less clear. Looking at MurmurHash( variable ) in code I think you could be easily forgiven for thinking that it was equivalent to MurmurHash h; h.append( variable ).

Copy link
Contributor Author

@proberts-cinesite proberts-cinesite Oct 17, 2022

Choose a reason for hiding this comment

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

The existing __repr__ function in the python binding (line 52 of MurmurHashBinding.cpp) seems to assume it already exists and produces IECore.MurmurHash( hash.toString() ) as the python representation of a MurmurHash object.

Changes Outdated
Comment on lines 4 to 7
Improvements
------------

- Added string constructor and `fromString` function to `IECore.MurmurHash`.
Copy link
Member

Choose a reason for hiding this comment

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

This is in the section for an already-released version, but needs to be in a new section. If you want the feature to be released in the next 10.4.x version, the PR also needs to be targeted to the RB-10.4 branch.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

retargeted to RB-10.4

Comment on lines 74 to 75
hs = IECore.MurmurHash()
hs.fromString( s )
Copy link
Member

Choose a reason for hiding this comment

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

I think static MurmurHash fromString(); might provide slightly better ergonomics, so this could just be a one-line hs = MurmurHash.fromString( s ).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

@proberts-cinesite proberts-cinesite force-pushed the MurmurHashStringCtor branch 2 times, most recently from d8b0a96 to af0c36e Compare October 17, 2022 13:38
@proberts-cinesite proberts-cinesite changed the base branch from main to RB-10.4 October 17, 2022 13:39
@proberts-cinesite proberts-cinesite force-pushed the MurmurHashStringCtor branch 12 times, most recently from 7d69223 to d55cbb9 Compare October 19, 2022 09:07
@johnhaddon
Copy link
Member

Thanks Paul. I'm still a bit uneasy about this string constructor, but at least the new sanity check and exception will catch most invalid uses. Could you squash everything down ready to merge please? You could also move the Improvements section about the Build section in Changes - we try to order things so the more interesting stuff is at the top...

@proberts-cinesite
Copy link
Contributor Author

all done

@johnhaddon johnhaddon merged commit 1be8465 into ImageEngine:RB-10.4 Oct 21, 2022
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