tock
style: spell out constants used in crypto interfaces
Pull Request Overview
This is inspired by the impending addition of yet one more _L constant. 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 _LEN in favor of _LENGTH following a general preference for len in 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 _L constants coming]
Documentation Updated
- [x] Updated the relevant files in
/docs, or no updates are required.
Formatting
- [ ] Ran
make prepush.
I personally don't mind the H + HL convention. I do think HASH_LEN is more clear. We also need to decide what to do in the digest and entropy HILs.
Here's what things look like if you expand H to HashKind and S to SignatureKind as well: https://github.com/tock/tock/compare/write-out-crypto-types-too?expand=1
I do favor that, though I cede the point that it is more verbose
Rebased and pulled in the changes from the branch which updates the types too.
We also need to decide what to do in the digest and entropy HILs.
I updated the digest HIL to match, I don't think there's anything relevant in the entropy HIL at the moment.
I don't think any type names are changing, just constants and type parameters (generics placeholders).
But yes, this now goes beyond constants, and thus the PR title / description no longer accurately matches this changeset.
I don't think any type names are changing, just constants and generics placeholders.
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?
Do you object to these changes at all, or just as part of this PR mixed in with the other changes?
Entirely; I thought we agreed that for better or worse that is Rust convention, and we use it pervasively throughout the kernel such that changing in one place just adds confusion and inconsistency.
In practice these constants H, S, etc. aren't widely used, they are just copied from trait impl to trait impl. The longer these generic setups get the more lines they take up and the harder they are to read, in places where the names aren't used.
https://github.com/tock/tock/blob/544b334bc51b3a0d888229b714911670b7c8aa65/boards/components/src/appid/checker_signature.rs#L83-L89
Where they are used, the explanation is two or three lines immediately above.
https://github.com/tock/tock/blob/544b334bc51b3a0d888229b714911670b7c8aa65/boards/components/src/appid/checker_signature.rs#L33-L44
I find, especially over time, it is easier to understand when they are a single letter, because now I don't have to guess is this a generic type, or a type imported from some library some where? Single letters are always generic types, and I know where to look for the definition. Longer types are actual types, and I can look for those.
~~This is not a hill I'll die on, so if you want it reverted, fine.~~
~~I'll just say that the names of the type parameters don't have to match the trait implementation from the trait definition. I agree that S: SignatureVerify is clear, but in other places where this bound does not exist, an assortment of letters (where we're lucky that they don't yet overlap) like H, S, ... is not particularly clear.~~
Edit: I think I was mostly irritated by the single letter type parameters in the macro definition, and I agree that in many other places, the ambiguity is removed by means of the trait bounds. So I'm fine with keeping it single-letter but don't feel strongly either way.
Also, seems like we're not the first debating this: https://users.rust-lang.org/t/one-character-generics/12317/14
I think I was mostly irritated by the single letter type parameters in the macro definition
macros are generally bad
I updated the whole thing to do both because after my comment with exemplar expanding everything there was one agreeing comment and then silence. I did say I updated both, but missed the PR title, sorry (somehow, my brain just always glazes over PR titles; I think I forget they are editable; I just use the comment thread as the current state of truth when reading PR updates. I fully acknowledge that is a weird thing to do and updating titles is probably useful for most folks).
On the merits of the question at hand, Rust naming convention for Type Parameters is concise UpperCamelCase, usually single uppercase letter: T.
Digging around some, it seems that the thread Leon link captures the pseudo-convention that seems to be the norm across Rust, Java, and C++ from current search results, i.e., single letter names for things that are truly generic types (Box<T>) and concise identifiers for generic types with some semantic restrictions (e.g., <Idx> in Rust core was an example linked from one discussion).
What do we think about H --> Hsh and S --> Sig?
I remember a conversation somewhere where we said the convention in both Rust and Tock is single letters for types but not for constants. I thought that was the understanding going in to this PR.
We use single letter types everywhere in Tock, I don't see why this is the PR to change that convention. I think H and S remain clear, particularly with the full description always within 5 lines of code. Plus, using single letter for types makes it clear that there is no actual type called Sig or Aes or Alm (for example) that the reader should look for. The only time we use single letters is with these generic types, so it is clear what they are and where they are defined. In the case we have name conflicts I think we use two upper case letters, again providing the same distinction.
I reverted (well, removed the commit) with the type name changes.
I squashed the digest commit in with the rest since it's all one logical change anyway.