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

Factor out ICheckSum and replace with System.Security.Cryptography.HashAlgorithm #255

Closed
clambertus opened this issue Dec 27, 2019 · 2 comments
Labels
help-wanted Extra attention is needed Lucene.Net Core pri:low up-for-grabs This issue is open to be worked on by anyone
Milestone

Comments

@clambertus
Copy link

IChecksum was brought over from Java in order to plug in the CRC32 class.

However, .NET already has its own abstract class, HashAlgorithm that is used to as a base class for all cryptographic algorithms. There are also several 3rd party implementations of CRC32 that implement HashAlgorithm that we could use (provided we drop support for .NET framework < 4.6.1). Crc32.NET claims to be one of the fastest implementations.

IChecksum is only utilized in a couple of places, but Lucene.NET's types should also be renamed accordingly to show they are using HashAlgorigthm instead of IChecksum:

  • BufferedChecksum > BufferedHashAlgorithm
  • BufferedChecksumIndexInput > BufferedHashAlgorithmIndexInput

The APIs between IChecksum and HashAlgorithm differ, but being that they serve the exact same purpose we should be able to make this work with a bit of refactoring.

JIRA link - [LUCENENET-637] created by nightowl888
@clambertus clambertus added Lucene.Net Core help-wanted Extra attention is needed up-for-grabs This issue is open to be worked on by anyone labels May 5, 2020
@NightOwl888 NightOwl888 added this to the Lucene.NET 4.8.0 milestone May 7, 2020
@NightOwl888
Copy link
Contributor

For now, IChecksum and its only implementation, BufferedChecksum have been marked internal. We will fix this before the release of Lucene.NET 4.8.0 if there is time, but it is only guaranteed to make it into the release if someone contributes it.

See #248 (comment) for details of what is required.

@NightOwl888
Copy link
Contributor

See #436. We are closing this due to the lack of benefit for the change.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
help-wanted Extra attention is needed Lucene.Net Core pri:low up-for-grabs This issue is open to be worked on by anyone
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants