-
-
Notifications
You must be signed in to change notification settings - Fork 238
match_class!
fallback branch is now optional for ()
#1246
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
API docs are being generated and will be shortly available at: https://godot-rust.github.io/docs/gdext/pr-1246 |
No plans for now, but we can leave it for some time until we see how
You might be able to turn this into a type-system error by explicitly writing something like: let () = $expr; Clippy should allow this as per rust-lang/rust-clippy#9048, but I haven't tested. Otherwise #[allow(clippy::let_unit_value)]
let _unit_value: () = $expr; |
Gotcha!
Huh? How could that happen when no rule matches that afaik? |
@@ -52,7 +52,7 @@ fn match_class_basic_mut_dispatch() { | |||
require_node(&node); | |||
2 | |||
}, | |||
_ => 3 // No comma. | |||
3 // No need for _, but we do need a value for this branch. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should definitely not support this.
If a fallback branch is needed, then _ =>
is totally acceptable to write.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Perfect
// No last block. | ||
($subject:ident, $($block:expr $(,)?)?) => {{ | ||
$($block)? | ||
}}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It seems like the case where the ?
is present is already handled, no?
Can this be written differently maybe -- to also outlaw the 3
syntax (see previous comment)?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah! That's what I was requesting to know, I already had a solution.
The things I can imagine are:
We need to find a way that's syntactically consistent and intuitive, otherwise I'd rather have explicit |
Right now it's either:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added a commit to test the empty match_class!
statement, which is just a special case of this syntax. Also cleaned up some other parts (like the comment you suggested).
Generally looks good, you can squash (feel free to remove my authorship), I'll give some others the chance to review 👍
Alright! Should we document it on the macro's doc comment? |
Yes, maybe update the sentence stating that the fallback is required -> only required if the arms have a type other than The code can probably be left as-is 🤔 |
568028b
to
019ed7a
Compare
Should be all good now unless there's some issue with the documentation wording. |
match_class! {}
macromatch_class!
fallback branch is now optional
match_class!
fallback branch is now optionalmatch_class!
fallback branch is now optional for ()
54e548c
to
bb5c924
Compare
Small changes to the doc, e.g. Once you're ready, feel free to squash 🙂 thanks for everything so far! |
Woops, totally right, my brain just registers it as no type type, my bad.
Thanks lots!
Yeah! That's what I thought too, and thus, this whole pr. Okay, squashing and commiting! |
bb5c924
to
03f45eb
Compare
All done! |
I commented on this on my previous PR, so I wanted to remove the need for it. I pr'd one approach, but the other one is the following:
I also wanted to comment on this comment that is still on the macro, are there any plans for it? Why was it left as a comment?
The reasons I'd advocate for removing the need for the last branch is because there are many use cases in which that bracket is just adding more verbosity. My concern is what would happen in a match that returns an int but no last bracket is used, then, what is the compile error? The one that appeared during testing is an error on the macro definition itself.