Match against an optional single trailing colon #1010
Conversation
Currently we allow multiple trailing colons when matching within the `check_format_non_negative` macro. We can be more restrictive with no loss of usability. Use `$(;)?` instead of `$(;)*` to match against 0 or 1 semi-colons instead of 0 or more.
| @@ -1640,7 +1640,7 @@ mod tests { | |||
|
|
|||
| // Creates individual test functions to make it easier to find which check failed. | |||
| macro_rules! check_format_non_negative { | |||
| ($denom:ident; $($test_name:ident, $val:expr, $format_string:expr, $expected:expr);* $(;)*) => { | |||
| ($denom:ident; $($test_name:ident, $val:expr, $format_string:expr, $expected:expr);* $(;)?) => { | |||
There was a problem hiding this comment.
@Kixunil, one of the other task items in edition 2018 checklist mentions using literal instead of expr. This is the location here, right? I can do the change but I cannot justify it. I get that literal is more restrictive than expr, what is the benefit of being more restrictive? Or is there some other benefit? Thanks.
(FTR the justification for this PR is a bit soft as well.)
There was a problem hiding this comment.
If one accidentally passes non-literal the compiler should produce better errors because it realizes the argument is wrong at the place where the macro was used - the error wouldn't come out of inside of the macro.
This PR is purely cosmetic to avoid potentially confusing some_macro!(a,b,c, ,,,,,,,);
There was a problem hiding this comment.
re the literal/expr thing, cheers, that makes good sense. I'll patch it.
re the cosmetic thing, I guessed as much. Is there a specif reason that you didn't do this (put the semi-colon inside the match pattern):
($denom:ident; $($test_name:ident, $val:expr, $format_string:expr, $expected:expr;)*) => {
Was it to be inline with how Rust allows for an optional comma? (Just out of interest and for my education.)
There was a problem hiding this comment.
expr -> literal change done in #1014
There was a problem hiding this comment.
Yes, trailing commas/semicolons are optional and putting it inside would make it mandatory.
Kixunil
added
trivial
…e trailing colon 88ce8fe Match against an optional single trailing colon (Tobin C. Harding) Pull request description: Currently we allow multiple trailing colons when matching within the `check_format_non_negative` macro. We can be more restrictive with no loss of usability. Use `$(;)?` instead of `$(;)*` to match against 0 or 1 semi-colons instead of 0 or more. Done as part of the [edition 2018 checklist](rust-bitcoin/rust-bitcoin#510). ACKs for top commit: Kixunil: ACK 88ce8fe apoelstra: ACK 88ce8fe Tree-SHA512: 4409c094f6a0aa49ddebdad850fd1d5a31a57dae8828f5a1db0ee5a855e1bce9e43aea69fa0b4d132068c3a43f1f62d35409b9ac5b32ed876e4dd586829e8e68
Currently we allow multiple trailing colons when matching within the
check_format_non_negativemacro. We can be more restrictive with noloss of usability.
Use
$(;)?instead of$(;)*to match against 0 or 1 semi-colonsinstead of 0 or more.
Done as part of the edition 2018 checklist.