target/riscv: allow hexadecimal values to expose_csr-like commands #1162
Conversation
| ++reg_address_str; | ||
| } | ||
| // try to detect if string starts with 0x | ||
| bool is_hex_address = strncmp(reg_address_str, "0x", 2) == 0 || |
There was a problem hiding this comment.
Why not simply a case sensitive comparison: strnicmp(reg_address_str, "0x", 2)?
There was a problem hiding this comment.
I could. I just forgot that this function existed. Will address
There was a problem hiding this comment.
actually no! This function does not exist in C language. This is Microsoft-specific extension.
There was a problem hiding this comment.
actually no! This function does not exist in C language. This is Microsoft-specific extension.
What about strncasecmp()?
There was a problem hiding this comment.
strcasecmp is not in the C or C++ standard. It's defined by POSIX.1-2001 and 4.4BSD. I would prefer not to use it.
src/target/riscv/riscv.c
Outdated
| bool is_hex_address = strncmp(reg_address_str, "0x", 2) == 0 || | ||
| strncmp(reg_address_str, "0X", 2) == 0; | ||
|
|
||
| unsigned chars_scanned; |
There was a problem hiding this comment.
Could use unsigned char instead of unsigned here but maybe coding style doesn't like that?
There was a problem hiding this comment.
this should be an unsigned int , actually. Writing unsigned is just an old habit of mine.
There was a problem hiding this comment.
@TommyMurphyTM1234 , given that there is a reaction from you under this thread, can I assume that this one is addressed from your point of view?
(Typically I would expect reviewer to resolve his threads, so I want to double-check that our expectations are aligned).
aap-sc
force-pushed
the
aap-sc/csr_as_hex
branch
4 times, most recently
from
4699b8e
to
6f88874
Compare
hexadecimal values are often used in the documentation. Forcing user to convert CSRs addresses to decimal is unnecessary.
aap-sc
force-pushed
the
aap-sc/csr_as_hex
branch
from
6f88874
to
92532db
Compare
|
I will be able to review this change by the end of the week. |
| /* For backward compatibility, allow multiple parameters within one TCL argument, separated by ',' */ | ||
| char *arg = strtok(args, ","); | ||
| while (arg) { | ||
| static bool parse_csr_address(const char *reg_address_str, unsigned int *reg_addr) |
There was a problem hiding this comment.
Could this function be replaced by existing parse_uint() from command.c?
There was a problem hiding this comment.
If i the description of those commands right - they still accept octal numbers. Given the nature of what we are dealing with here - I would forbid usage of octals to reduce risk of errors...
Though, now that I've said it - I think it would not hurt to add an actual check that the number does not start with zero.
There was a problem hiding this comment.
Good point about the octals. Better to leave these unsupported to avoid user mistakes.
I think it would not hurt to add an actual check that the number does not start with zero.
Sounds good.
| /* For backward compatibility, allow multiple parameters within one TCL | ||
| * argument, separated by ',' */ | ||
| for (char *arg = strtok(args, ","); arg; arg = strtok(NULL, ",")) { |
There was a problem hiding this comment.
(Just a comment, no change requested)
In future, it would be my preference to simplify the syntax of expose_csrs and expose_custom. I'd deprecate (and eventually remove) the syntax with comma-separated list of registers.
No description provided.