Re: [Pharo-project] CRC in System-Hashing?
On Mar 2, 2012, at 8:19 AM, Max Leske wrote:
I need to calculate CRC for Git and couldn't find it in the system packages (I guess I'd find it in Cryptography). Since Pharo has a system package called "System-Hashing" I was wondering if anyone sees a reason why CRC should not be part of that package. That would also allow the Zip implementation to use the system CRC function (at the moment Zip implements its own CRC check).
Yes, that would be nice. But people will yell at you for making Pharo bloated and going in the completely wrong direction and that you should and never ever add any code.
Another thing: going through Sytem-Hashing I noticed that there are two implementations for SHA1: "SHA1" and "SecureHashAlgorithm". They both have the exact same class comment but vary slightly in the messages they implement (although most of them are present in both classes). Can we remove one?
Yes. But for sure someone will yell at you if you remove it, because the system should be stable and not force people to change their code. Marcus -- Marcus Denker -- http://marcusdenker.de
On Mar 2, 2012, at 9:05 AM, Marcus Denker wrote:
On Mar 2, 2012, at 8:19 AM, Max Leske wrote:
I need to calculate CRC for Git and couldn't find it in the system packages (I guess I'd find it in Cryptography). Since Pharo has a system package called "System-Hashing" I was wondering if anyone sees a reason why CRC should not be part of that package. That would also allow the Zip implementation to use the system CRC function (at the moment Zip implements its own CRC check).
Yes, that would be nice. But people will yell at you for making Pharo bloated and going in the completely wrong direction and that you should and never ever add any code.
Decomposing != Bloating. As long as you refactor the current users (which currently use the implementation in ZipWriteStream) to use the new CRC class, it should be fine. I would suggest deprecating the class method in ZipWriteStream rather than removing it though, based on the users in image, it is a likely method to be in use by external packages doing CRC calculations. It also seems the primitive for updating CRC is not reliant on the CrcTable class var, so should be safe to use in new class' method as well.
Another thing: going through Sytem-Hashing I noticed that there are two implementations for SHA1: "SHA1" and "SecureHashAlgorithm". They both have the exact same class comment but vary slightly in the messages they implement (although most of them are present in both classes). Can we remove one?
Yes. But for sure someone will yell at you if you remove it, because the system should be stable and not force people to change their code.
IIRC, it was initially included as SecureHashAlgorithm, being renamed when imported from the Cryptography package. To allow existing packages relying on Crypto to load cleanly, SHA1 was later also included. As there are already other name clashes with the Crypto package (MD5 for instance) I would vote for deprecating SecureHashAlgorithm. (SHA1 is arguably a more descriptive name anyways) Not sure if it's worth including hashMessage: in SHA1, it's just a utility method. Cheers, Henry
Thanks for the responses. I'll wait a little longer and see if Rob has anything to say about this. Otherwise I will do what Henrik suggests: deprecate SecureHashAlgorithm (why not remove it completely?) refactor users of SecureHashAlogrithm to use SHA1 instead add CRC to SystemHashing (possibly a copy from Cryptography) refactor the Zip implementation to use the new CRC class deprecate crc methods in Zip implementation I actually like #hashMessage: better than #hashStream:. #hashStream: says to my "I expect a stream as my argument" where in reality it will take a string or a ByteArray (neither of which is obviously a streamâ¦). What I found irritating though was that one has to send #new to SecureHashAlgorithm first before hashing. So I propose to keep #hashMessage: but to move it to the class side of SHA1: SHA1 class>>hashMessage: aStringOrByteArray ^ self new hashStream: aStringOrByteArray; yourself Cheers, Max On 02.03.2012, at 09:46, Henrik Johansen wrote:
On Mar 2, 2012, at 9:05 AM, Marcus Denker wrote:
On Mar 2, 2012, at 8:19 AM, Max Leske wrote:
I need to calculate CRC for Git and couldn't find it in the system packages (I guess I'd find it in Cryptography). Since Pharo has a system package called "System-Hashing" I was wondering if anyone sees a reason why CRC should not be part of that package. That would also allow the Zip implementation to use the system CRC function (at the moment Zip implements its own CRC check).
Yes, that would be nice. But people will yell at you for making Pharo bloated and going in the completely wrong direction and that you should and never ever add any code.
Decomposing != Bloating. As long as you refactor the current users (which currently use the implementation in ZipWriteStream) to use the new CRC class, it should be fine. I would suggest deprecating the class method in ZipWriteStream rather than removing it though, based on the users in image, it is a likely method to be in use by external packages doing CRC calculations.
It also seems the primitive for updating CRC is not reliant on the CrcTable class var, so should be safe to use in new class' method as well.
Another thing: going through Sytem-Hashing I noticed that there are two implementations for SHA1: "SHA1" and "SecureHashAlgorithm". They both have the exact same class comment but vary slightly in the messages they implement (although most of them are present in both classes). Can we remove one?
Yes. But for sure someone will yell at you if you remove it, because the system should be stable and not force people to change their code.
IIRC, it was initially included as SecureHashAlgorithm, being renamed when imported from the Cryptography package. To allow existing packages relying on Crypto to load cleanly, SHA1 was later also included.
As there are already other name clashes with the Crypto package (MD5 for instance) I would vote for deprecating SecureHashAlgorithm. (SHA1 is arguably a more descriptive name anyways) Not sure if it's worth including hashMessage: in SHA1, it's just a utility method.
Cheers, Henry
Sounds good to me. Now for 1.5 alpha :) Stef On Mar 2, 2012, at 10:21 AM, Max Leske wrote:
Thanks for the responses.
I'll wait a little longer and see if Rob has anything to say about this. Otherwise I will do what Henrik suggests: ⢠deprecate SecureHashAlgorithm (why not remove it completely?) ⢠refactor users of SecureHashAlogrithm to use SHA1 instead ⢠add CRC to SystemHashing (possibly a copy from Cryptography) ⢠refactor the Zip implementation to use the new CRC class ⢠deprecate crc methods in Zip implementation
I actually like #hashMessage: better than #hashStream:. #hashStream: says to my "I expect a stream as my argument" where in reality it will take a string or a ByteArray (neither of which is obviously a streamâ¦). What I found irritating though was that one has to send #new to SecureHashAlgorithm first before hashing. So I propose to keep #hashMessage: but to move it to the class side of SHA1:
SHA1 class>>hashMessage: aStringOrByteArray ^ self new hashStream: aStringOrByteArray; yourself
Cheers, Max
On 02.03.2012, at 09:46, Henrik Johansen wrote:
On Mar 2, 2012, at 9:05 AM, Marcus Denker wrote:
On Mar 2, 2012, at 8:19 AM, Max Leske wrote:
I need to calculate CRC for Git and couldn't find it in the system packages (I guess I'd find it in Cryptography). Since Pharo has a system package called "System-Hashing" I was wondering if anyone sees a reason why CRC should not be part of that package. That would also allow the Zip implementation to use the system CRC function (at the moment Zip implements its own CRC check).
Yes, that would be nice. But people will yell at you for making Pharo bloated and going in the completely wrong direction and that you should and never ever add any code.
Decomposing != Bloating. As long as you refactor the current users (which currently use the implementation in ZipWriteStream) to use the new CRC class, it should be fine. I would suggest deprecating the class method in ZipWriteStream rather than removing it though, based on the users in image, it is a likely method to be in use by external packages doing CRC calculations.
It also seems the primitive for updating CRC is not reliant on the CrcTable class var, so should be safe to use in new class' method as well.
Another thing: going through Sytem-Hashing I noticed that there are two implementations for SHA1: "SHA1" and "SecureHashAlgorithm". They both have the exact same class comment but vary slightly in the messages they implement (although most of them are present in both classes). Can we remove one?
Yes. But for sure someone will yell at you if you remove it, because the system should be stable and not force people to change their code.
IIRC, it was initially included as SecureHashAlgorithm, being renamed when imported from the Cryptography package. To allow existing packages relying on Crypto to load cleanly, SHA1 was later also included.
As there are already other name clashes with the Crypto package (MD5 for instance) I would vote for deprecating SecureHashAlgorithm. (SHA1 is arguably a more descriptive name anyways) Not sure if it's worth including hashMessage: in SHA1, it's just a utility method.
Cheers, Henry
On 2 March 2012 10:21, Max Leske <maxleske@gmail.com> wrote:
Thanks for the responses.
I'll wait a little longer and see if Rob has anything to say about this. Otherwise I will do what Henrik suggests:
deprecate SecureHashAlgorithm (why not remove it completely?) refactor users of SecureHashAlogrithm to use SHA1 instead add CRC to SystemHashing (possibly a copy from Cryptography) refactor the Zip implementation to use the new CRC class deprecate crc methods in Zip implementation
I actually like #hashMessage: better than #hashStream:. #hashStream: says to my "I expect a stream as my argument" where in reality it will take a string or a ByteArray (neither of which is obviously a streamâ¦). What I found irritating though was that one has to send #new to SecureHashAlgorithm first before hashing. So I propose to keep #hashMessage: but to move it to the class side of SHA1:
SHA1 class>>hashMessage: aStringOrByteArray ^ self new hashStream: aStringOrByteArray; yourself
hmm.. #hashMessage: sounds a bit weird to my taste. maybe #hashCollection: then?
Cheers, Max
-- Best regards, Igor Stasenko.
Oops! #hashStream: *does* actually take a stream⦠#hashMessage: takes a string or a ByteArray and sends #readStream to the argument. So: different proposal: drop #hashMessage: completely. Instead add: SHA1 class>>hashStream: aPositionableStream ^ self new hashStream: aPositionableStream; yourself Thanks Igor for unintentionally bumping me in the head :) On 02.03.2012, at 15:03, Igor Stasenko wrote:
On 2 March 2012 10:21, Max Leske <maxleske@gmail.com> wrote:
Thanks for the responses.
I'll wait a little longer and see if Rob has anything to say about this. Otherwise I will do what Henrik suggests:
deprecate SecureHashAlgorithm (why not remove it completely?) refactor users of SecureHashAlogrithm to use SHA1 instead add CRC to SystemHashing (possibly a copy from Cryptography) refactor the Zip implementation to use the new CRC class deprecate crc methods in Zip implementation
I actually like #hashMessage: better than #hashStream:. #hashStream: says to my "I expect a stream as my argument" where in reality it will take a string or a ByteArray (neither of which is obviously a streamâ¦). What I found irritating though was that one has to send #new to SecureHashAlgorithm first before hashing. So I propose to keep #hashMessage: but to move it to the class side of SHA1:
SHA1 class>>hashMessage: aStringOrByteArray ^ self new hashStream: aStringOrByteArray; yourself
hmm.. #hashMessage: sounds a bit weird to my taste. maybe #hashCollection: then?
Cheers, Max
-- Best regards, Igor Stasenko.
Sorry, I couldn't resist :$ On Fri, Mar 2, 2012 at 6:21 AM, Max Leske <maxleske@gmail.com> wrote:
Thanks for the responses.
I'll wait a little longer and see if Rob has anything to say about this. Otherwise I will do what Henrik suggests:
- deprecate SecureHashAlgorithm (why not remove it completely?) - refactor users of SecureHashAlogrithm to use SHA1 instead
http://code.google.com/p/pharo/issues/detail?id=5427&thanks=5427&ts=13306994...
- add CRC to SystemHashing (possibly a copy from Cryptography) - refactor the Zip implementation to use the new CRC class - deprecate crc methods in Zip implementation
http://code.google.com/p/pharo/issues/detail?id=5426&thanks=5426&ts=13306992... Guille
I actually like #hashMessage: better than #hashStream:. #hashStream: says to my "I expect a stream as my argument" where in reality it will take a string or a ByteArray (neither of which is obviously a streamâ¦). What I found irritating though was that one has to send #new to SecureHashAlgorithm first before hashing. So I propose to keep #hashMessage: but to move it to the class side of SHA1:
SHA1 class>>hashMessage: aStringOrByteArray ^ self new hashStream: aStringOrByteArray; yourself
Cheers, Max
On 02.03.2012, at 09:46, Henrik Johansen wrote:
On Mar 2, 2012, at 9:05 AM, Marcus Denker wrote:
On Mar 2, 2012, at 8:19 AM, Max Leske wrote:
I need to calculate CRC for Git and couldn't find it in the system packages (I guess I'd find it in Cryptography). Since Pharo has a system package called "System-Hashing" I was wondering if anyone sees a reason why CRC should not be part of that package. That would also allow the Zip implementation to use the system CRC function (at the moment Zip implements its own CRC check).
Yes, that would be nice. But people will yell at you for making Pharo bloated and going in the completely wrong direction and
that you should and never ever add any code.
Decomposing != Bloating. As long as you refactor the current users (which currently use the implementation in ZipWriteStream) to use the new CRC class, it should be fine. I would suggest deprecating the class method in ZipWriteStream rather than removing it though, based on the users in image, it is a likely method to be in use by external packages doing CRC calculations.
It also seems the primitive for updating CRC is not reliant on the CrcTable class var, so should be safe to use in new class' method as well.
Another thing: going through Sytem-Hashing I noticed that there are two implementations for SHA1: "SHA1" and "SecureHashAlgorithm". They both have the exact same class comment but vary slightly in the messages they implement (although most of them are present in both classes). Can we remove one?
Yes. But for sure someone will yell at you if you remove it, because the system should be stable and not force people to
change their code.
IIRC, it was initially included as SecureHashAlgorithm, being renamed when imported from the Cryptography package. To allow existing packages relying on Crypto to load cleanly, SHA1 was later also included.
As there are already other name clashes with the Crypto package (MD5 for instance) I would vote for deprecating SecureHashAlgorithm. (SHA1 is arguably a more descriptive name anyways) Not sure if it's worth including hashMessage: in SHA1, it's just a utility method.
Cheers, Henry
Marcus Do you play the role of yellers :) Let them do and we should move on. So max if you have solution for a better world we are all ears open :) Stef
I need to calculate CRC for Git and couldn't find it in the system packages (I guess I'd find it in Cryptography). Since Pharo has a system package called "System-Hashing" I was wondering if anyone sees a reason why CRC should not be part of that package. That would also allow the Zip implementation to use the system CRC function (at the moment Zip implements its own CRC check).
Yes, that would be nice. But people will yell at you for making Pharo bloated and going in the completely wrong direction and that you should and never ever add any code.
Another thing: going through Sytem-Hashing I noticed that there are two implementations for SHA1: "SHA1" and "SecureHashAlgorithm". They both have the exact same class comment but vary slightly in the messages they implement (although most of them are present in both classes). Can we remove one?
Yes. But for sure someone will yell at you if you remove it, because the system should be stable and not force people to change their code.
Marcus
-- Marcus Denker -- http://marcusdenker.de
On 2 March 2012 09:05, Marcus Denker <marcus.denker@inria.fr> wrote:
On Mar 2, 2012, at 8:19 AM, Max Leske wrote:
I need to calculate CRC for Git and couldn't find it in the system packages (I guess I'd find it in Cryptography). Since Pharo has a system package called "System-Hashing" I was wondering if anyone sees a reason why CRC should not be part of that package. That would also allow the Zip implementation to use the system CRC function (at the moment Zip implements its own CRC check).
Yes, that would be nice. But people will yell at you for making Pharo bloated and going in the completely wrong direction and that you should and never ever add any code.
Another thing: going through Sytem-Hashing I noticed that there are two implementations for SHA1: "SHA1" and "SecureHashAlgorithm". They both have the exact same class comment but vary slightly in the messages they implement (although most of them are present in both classes). Can we remove one?
Yes. But for sure someone will yell at you if you remove it, because the system should be stable and not force people to change their code.
ROFL :)
    Marcus
-- Marcus Denker -- http://marcusdenker.de
-- Best regards, Igor Stasenko.
participants (6)
-
Guillermo Polito -
Henrik Johansen -
Igor Stasenko -
Marcus Denker -
Max Leske -
Stéphane Ducasse