Skip to content
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

Simplify Slice File Hashing Logic #701

Merged

Conversation

InsertCreativityHere
Copy link
Member

This PR simplifies the logic used to compute hashes of our Slice files.
No more extension trait, with multiple implementations; just one function.

The only change in logic is that previously, we would compute a hash for each file separately, then hash all those hashes together:

total_hash = hash(hash(file1), hash(file2), hash(file3), ...)  // old

Now, we just have a single hash engine, which we directly feed all the files into:

total_hash = hash(file1, file2, file3, ...)  // new

I can't think of any adverse effects to this simpler approach, compared to the previous nested-hashing approach.

}
}
// Filter out any reference files, and sort the source files which remain.
let mut sorted_sources: Vec<&SliceFile> = files.iter().filter(|f| f.is_source).collect();
Copy link
Member

Choose a reason for hiding this comment

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

I forget where were landed on the discussion of only including src files. Was this previously being done elsewhere?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, you're right I should of added a comment mentioning this.
Currently, we do the filtering in slicec-cs:

let src_files: Vec<&SliceFile> = compilation_state.files.iter().filter(|f| f.is_source).collect();
let hash = SliceFile::compute_sha256_hash(&src_files);

https://github.com/icerpc/icerpc-csharp/blob/9edecc0ccaddee829828f2da64ce150b692d340f/tools/slicec-cs/src/main.rs#L65-L67
My thinking was to move it in slicec, to keep all the hashing logic and what/how we do it in a single place.


I'm also not completely sold on one vs the other, but this is the current behavior we have, so I kept it.
I'll blog about it, and see if we can reach a conclusion.

Copy link
Member

@pepone pepone left a comment

Choose a reason for hiding this comment

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

Much simpler, nice!

@InsertCreativityHere InsertCreativityHere merged commit 41012e2 into icerpc:main Sep 10, 2024
4 checks passed
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.

3 participants