style: spell out constants used in crypto interfaces #4443
Conversation
|
I personally don't mind the |
|
Here's what things look like if you expand I do favor that, though I cede the point that it is more verbose |
There was a problem hiding this comment.
I think this is a solid improvement.
While it is generally [*] convention to use single letters for trait bounds
I believe I've seen a fair share of examples where trait bounds were camel-case identifiers, esp. where there is ambiguity. In the diff most trait implementations or type definitions that contain generic trait bounds are split over multiple lines anyways, so I'd be totally happy to see these be expanded as well.
But this alone is a good improvement in on itself, and I don't feel particularly strongly about the single-letter trait bounds.
In accordance with the [Style Guide](doc/Style.md#using-descriptive-names), replace the two-letter constants with spelled out versions for `HASH_LEN` and `SIGNATURE_LEN` in crypto interfaces. `_LEN` is chosen in favor of `_LENGTH` following a general prefernce for `len` in Rust.
eef0110 to
7af2dbe
Compare
|
Rebased and pulled in the changes from the branch which updates the types too.
I updated the digest HIL to match, I don't think there's anything relevant in the entropy HIL at the moment. |
| SignatureKind, | ||
| HashKind, |
There was a problem hiding this comment.
Might be nitpicky, but these fields don't really refer to a SignatureKind "marker" as much as they refer to a Signer "component" that actually does work, right? Same for HashKind -> Hasher?
I'm slightly confused by the Kind part in the generic's name, but it's not a big deal either way.
There was a problem hiding this comment.
This is a good nitpick to discuss, the whole point is clarity from more explicit names.
This is also definitely a case of Pat might be learning Rust via PR discussions... but my rationale for the current naming was that, IIUC, this is ultimately the placeholder label for a type. E.g., looking at the documentation explaining bounds this would be a "generic type SignatureKind". I used "Kind" in lieu of "Type" as I thought that was a common convention for avoiding complexity around "Type" as a keyword, though maybe it is not needed here.
Would SignerType (and HasherType) make more sense?
There was a problem hiding this comment.
Ah, yes, this clears up some confusion on my end. In short:
Would
SignerType(andHasherType) make more sense?
Yes, but I think the Type-part is redundant.
While type is massively overloaded, I don't believe kind qualifies as a good substitute in this case. When I read SignatureKind, I'm thinking of "Ed25519 vs. RSA Signatures". Which is kind of what we're allowing to be switched out here, except it misses the point. The generic placeholder name will be filled in with an actual component Type computing a signature of a given "kind", and not just a Type that indicates a "kind" of an asymmetric signature. (re-reading this, it's still confusing, sorry!)
As for Type being overloaded: I think it's fairly obvious to most experienced Rust developers that these CamelCase placeholders (perhaps maybe less so than non-single-character names) are generic placeholders for types. These are also easily distinguishable from const generics, which are conventionally spelled out in MACRO_CASE. Adding an explicit Type or Kind suffix, to me, confuses more than it helps.
|
I don't think any type names are changing, just constants and generics placeholders. But yes, this now goes beyond constants, and thus the PR title / description no longer accurately matches this changeset. |
I guess I'm referring to the generics placeholders, which I don't think we should start changing now. |
|
Do you object to these changes at all, or just as part of this PR mixed in with the other changes? |
Pull Request Overview
This is inspired by the impending addition of yet one more
_Lconstant. I was going to suggest writing things out as a clearer alternative on that PR, but realized it was really just following the existing style.While it is generally [*] convention to use single letters for trait bounds, the same isn't quite as universal for const generics. Our Style Guide prefers descriptive names. This feels like a use case where there isn't huge overhead in physical space on the screen from writing these out, and there is much more clarity in reading standalone use, especially as uses get further from the defining/explaining site in the HIL. I went
_LENin favor of_LENGTHfollowing a general preference forlenin Rust.[*] In my ever-so-humble-opinion, a terrible convention
Testing Strategy
This pull request was tested by compiling.
TODO or Help Wanted
Vibes check? Am I just being curmudgeonly about a Rust-ism with single-letter nonsense, or do others also feel this improves the readability? [Again, especially with an expectation of yet more
_Lconstants coming]Documentation Updated
/docs, or no updates are required.Formatting
make prepush.