Digest consolidation #4037
Digest consolidation #4037
Conversation
Papierkorb
force-pushed
the
md5-to-digest
branch
from
f4055b0
to
c4691d5
Compare
There was a problem hiding this comment.
I'm wary of ValueRef. It's unsafe since it keeps a pointer to a stack allocated value. It's good for speed, but the pointer is passed, not kept internally, so the unsafety leaks.
If we eventually require unsafe blocks (as Rust), then accessing ValueRef will be unsafe too, and the block API will be ugly/painful to use.
Maybe dropping the block form would be simpler?
src/digest/base.cr
Outdated
| require "value_ref" | ||
| require "base64" | ||
|
|
||
| module Digest::Base(C) |
There was a problem hiding this comment.
Please use T for denoting a generic.
There was a problem hiding this comment.
Why? Naming standards we all agree on (i.e. dont use 1 letter variable names) seem to get routinely ignored when using generics. Wouldn't ContextType be a better name? T is a good generic name for generic containers, this is not a generic container.
There was a problem hiding this comment.
Convention.
Reading C.new below made we wondered what the hell it was, and why someone would call a struct/class like that, until I noticed it denoted a generic. Reading ContextType I would never guess it was a generic.
Reading T I immediately understand it as a generic type.
There was a problem hiding this comment.
I think it's an awful shame to add the feature so that our generic variables can have descriptive names, but not use it. Non-generic type names tend not to end in Type for obvious reasons, but if we made it the convention that multi-char generic names ended in Type would that make it more readable for you?
The block form allows one to calculate a hash of large data easily, without having everything in memory. Use-case would be hashing large files. Edit I'd rather have a |
Papierkorb
force-pushed
the
md5-to-digest
branch
from
c4691d5
to
3e8933b
Compare
|
I'm not entirely sure about the current design of digest. Providing only the block form is not good, we should provide a way to create a digest, maybe store it in an instance variable and update it as data comes. For this we need to use a class. But writing |
Papierkorb
force-pushed
the
md5-to-digest
branch
from
3e8933b
to
780dc7b
Compare
That would've been inherently unsafe too,
Ah, that sounds really good! In that case, I don't have much qualms about using a
Agreed. Latest changes: Removed |
|
I'm also not sure we should design this in a PR. For example there's no reason to use a generic module here. We'll some time in the future design this and get back to this. |
Papierkorb
force-pushed
the
md5-to-digest
branch
from
780dc7b
to
6e2eb6a
Compare
|
So while we wait for the CI to finish, the current PR contains:
|
|
Merged, thanks for the contribution @Papierkorb! |
Hello,
Here's the Digest consolidation as promised in #4021 :)
The original MD5 implementation used a public
Contextclass and a privateContextImplstruct. I guess the original creators intention was to save a memory allocation in the non-yielding digest methods, and use memory allocation only for the yielding digest methods.After a discussion in IRC, we came to the conclusion to further improve upon it by make it not use any dynamic memory allocation. As this is not the first time I needed something like this (And likely, others did too), we agreed to put a generic wrapper into the stdlib, called
ValueRef(T). This wrapper basically mimics a C++ variable reference.I'm eager to hear y'all opinions, if this is ok, or if the
ValueRef(T)should not be there at all, or be part of its own PR :)(Side note: with that there are now 5000 stdlib unit tests. Yay)