Add UL/ULL suffixes to constants in encoding.h#398
Conversation
There was a problem hiding this comment.
OK with me after my comment is addressed, but I'd like feedback from others before merging.
Thanks for the contribution, @heathdutton.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #398 +/- ##
==========================================
+ Coverage 96.53% 97.08% +0.55%
==========================================
Files 10 14 +4
Lines 750 926 +176
==========================================
+ Hits 724 899 +175
- Misses 26 27 +1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
66c4f2f to
0669e38
Compare
aswaterman
left a comment
There was a problem hiding this comment.
Should we not wrap all of the remaining numerical constants (the ones that fit within 32 bits) with _RISCV_CSR_UL?
0669e38 to
819db49
Compare
|
Done - wrapped all 32-bit constants with |
Timmmm
left a comment
There was a problem hiding this comment.
LGTM. I guess the 32-bit version isn't strictly necessary (I think it would only make a difference on 16-bit CPUs) but it's nice for symmetry. Also if it is necessary then there are some 32-bit valeus in the generated code that maybe should have this applied too.
Also I'd maybe rename to just _RISCV_UL, _RISCV_ULL since the macros aren't really CSR-specific. Very minor though.
819db49 to
5f120ba
Compare
|
Renamed to |
|
@Timmmm it isn't only about the width; it's also about the signedness. For example, |
Adds
_ULand_ULLmacros (following the Linux kernel pattern) and wraps all 64-bit constants inencoding.hwith_ULL().This ensures the constants have the correct type in C while remaining compatible with assembly code.
Fixes #173