Implemented new functions to format literals as seperated hex idents #134
Conversation
therealprof
force-pushed
the
hex-literals
branch
from
3dab765 to
5fc8b76
Compare
|
@japaric Mind having a look? |
There was a problem hiding this comment.
Thanks for the PR, @therealprof.
I agree this is an improvement for "big" 32 bit literals like addresses but not so much for small numbers (<100) so I have proposed some tweaks. What do you think?
I'd be interested in hearing other people's opinions as well. @jonas-schievink, care to chime in?
src/generate.rs
Outdated
|
|
||
| if let Some(cpu) = d.cpu.as_ref() { | ||
| let bits = util::unsuffixed(cpu.nvic_priority_bits as u64); | ||
| let bits = util::hex(cpu.nvic_priority_bits); |
There was a problem hiding this comment.
I don't think writing this one in hex is going to make it more readable as the value is always in the range from 1 to 8.
There was a problem hiding this comment.
I prefer to have everything base 2/16 (as implied by the word "bits" here) related written in hex but I do agree it makes little sense for small numbers. I can get rid of that.
src/generate.rs
Outdated
| mask: util::hex_or_bool((((1 as u64) << width) - 1) as u32, width), | ||
| name: &f.name, | ||
| offset: util::unsuffixed(u64(f.bit_range.offset)), | ||
| offset: util::hex(f.bit_range.offset), |
There was a problem hiding this comment.
This value will in the range 0..32. TBH, I personally would find it easier to read it in decimal since it appears in the RHS of expressions like value << offset.
There was a problem hiding this comment.
I agree, will change that back.
There was a problem hiding this comment.
@japaric Actually I can't change that back because offset now takes a Token while util::unsuffixed() produces a Lit. If you want I can add some more code to produce a decimal token to be fed into offset.
There was a problem hiding this comment.
Or, maybe, unsuffixed could return Tokens.
There was a problem hiding this comment.
Okay, it's done.
src/util.rs
Outdated
| } | ||
|
|
||
| /// Turns `n` into an unsuffixed separated hex ident | ||
| pub fn hex(n: u32) -> Ident { |
There was a problem hiding this comment.
nit: Ident is for (parsed) identifiers. I see that there's no Lit variant for hexadecimal so we can't use that. I think returning Tokens would be the most correct choice after Lit. Could you change this to Tokens? (I think you would have to do something like let mut ts = Tokens::new(); ts.append("0xdead_beef"); ts
There was a problem hiding this comment.
I will try that.
|
The general idea makes sense to me, as do @japaric's comments. I don't know enough about ARM to have any real insight here :) |
|
☔ The latest upstream changes (presumably #121) made this pull request unmergeable. Please resolve the merge conflicts. |
The two main reasons behind this change are ensuring that the numbers (mostly addresses, masks and default values) line up with the datasheet for easier searching and verification since those are pretty much always expressed in hexadecimal. The other reason being clippy now complains about long numeric literals (without seperators) being hard to read -- I agree. Signed-off-by: Daniel Egger <daniel@eggers-club.de>
therealprof
force-pushed
the
hex-literals
branch
from
5fc8b76 to
e306adf
Compare
|
@japaric Except for the one change requested which I commented above the other requests have been addressed now. |
src/util.rs
Outdated
| /// Turns `n` into an unsuffixed token | ||
| pub fn unsuffixed(n: u64) -> Tokens { | ||
| let mut t = Tokens::new(); | ||
| t.append (format!("{}", n)); |
There was a problem hiding this comment.
nit: stray whitespace between append and (
There was a problem hiding this comment.
You'll find a lot more nits if you run rustfmt over it, which is preventing me from doing it all the time. 😉
This changes the util::unsuffixed() and util::unsuffixed_or_bool() functions to return a quote::Tokens instead of a syn::Lit. Signed-off-by: Daniel Egger <daniel@eggers-club.de>
therealprof
force-pushed
the
hex-literals
branch
from
01c440a to
b5756e8
Compare
|
Thanks, @therealprof. @homunkulus r+ |
|
📌 Commit b5756e8 has been approved by |
|
💔 Test failed - status-travis |
|
☀️ Test successful - status-appveyor, status-travis |
4 participants
The two main reasons behind this change are ensuring that the numbers
(mostly addresses, masks and default values) line up with the datasheet
for easier searching and verification since those are pretty much always
expressed in hexadecimal. The other reason being clippy now complains
about long numeric literals (without seperators) being hard to read -- I
agree.
Signed-off-by: Daniel Egger daniel@eggers-club.de