Skip to content

Conversation

@Rob-Hague
Copy link
Collaborator

While the library is not really so modern as to think about trimming and AOT, with the default behaviour to trim all assemblies when trimming an application we have seen issues such as #1167, and I believe #859 (comment) is related.

Fortunately it does not require many changes.

{
return _hash.ComputeHash(input);
#if !NET462
using var hash = IncrementalHash.CreateHash(_hashAlgorithmName);
Copy link
Member

Choose a reason for hiding this comment

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

Would we benefit (performance wise) from creating the hash in the ctor and reusing it here.
Please create a small benchmark to help us decide.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

In addition to my other comment: this is a correctness fix, not a performance improvement. I would rather work on other things than to run a benchmark for such a change. Sorry for being blunt.

Copy link
Collaborator

Choose a reason for hiding this comment

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

So maybe let's move the hash creating to the constructor. What do you think? this is the last change and I will be able to merge it.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Makes sense. Done

@WojciechNagorski
Copy link
Collaborator

I will review this next week.

{
return _hash.ComputeHash(input);
#if !NET462
using var hash = IncrementalHash.CreateHash(_hashAlgorithmName);
Copy link
Collaborator

Choose a reason for hiding this comment

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

So maybe let's move the hash creating to the constructor. What do you think? this is the last change and I will be able to merge it.

Copy link
Collaborator

@WojciechNagorski WojciechNagorski left a comment

Choose a reason for hiding this comment

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

Now LGTM!

@WojciechNagorski WojciechNagorski merged commit 64b428f into sshnet:develop Jan 26, 2024
@Rob-Hague Rob-Hague deleted the trimming branch January 26, 2024 08:59
@WojciechNagorski
Copy link
Collaborator

This issue has been fixed in the 2024.0.0 version.

@WojciechNagorski WojciechNagorski added this to the 2024.0.0 milestone Feb 22, 2024
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