Skip to content
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

Reduce boilerplate with the matches! macro #76995

Merged
merged 1 commit into from
Oct 6, 2020

Conversation

LingMan
Copy link
Contributor

@LingMan LingMan commented Sep 21, 2020

Replaces simple bool matches of the form

match $expr {
    $pattern => true
    _ => false
}

and their inverse with invocations of the matches! macro.

Limited to rustc_middle for now to get my feet wet.

Replaces simple bool `match`es of the form

    match $expr {
        $pattern => true
        _ => false
    }

and their inverse with invocations of the matches! macro.
@rust-highfive
Copy link
Collaborator

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @varkor (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

Please see the contribution instructions for more information.

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Sep 21, 2020
@tesuji
Copy link
Contributor

tesuji commented Sep 21, 2020

Does this affect compile time of compiler?

@tmandry
Copy link
Member

tmandry commented Sep 21, 2020

@lzutao It's only a macro expansion, I doubt it would have any measurable effect on the compiler build.

@LingMan
Copy link
Contributor Author

LingMan commented Sep 21, 2020

Since macro expansion already happens with ./x.py check, I did this to get an idea:

$ git switch master
$ ./x.py check
[snip, just to fill the cache]
$ git switch middle_matches
$ ./x.py check
Build completed successfully in 0:01:26
$ git switch master
$ ./x.py check
Build completed successfully in 0:01:26

Not perfectly scientific but hopefully this is enough to address your compile time concerns.

@jyn514 jyn514 added C-cleanup Category: PRs that clean code up or issues documenting cleanup. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Sep 22, 2020
@Mark-Simulacrum
Copy link
Member

I don't think we should worry about trivial compile time +/- like a few macros until we start tracking it (hopefully soon!).

I felt that the patch as a whole improved readability, and though I am not 100% sure we want to use matches! whenever a boolean output is expected, I think in the cases provided here it made sense to do so for the most part. I was uncertain if we should do so when we want !matches!(exp, pat), because I personally find the not blends in for me -- curious to here what others think. I think when looking at a particular case, it's not bad, but it does feel like it blends in a bit (in particular since there's already a ! sigil right after the macro).

cc @rust-lang/libs if y'all are interested in seeing some real-world examples of matches use or have thoughts on the !matches!(exp, pat) pattern (easily searched for in the diff if you want).

@varkor
Copy link
Member

varkor commented Sep 22, 2020

These all seem like improvements to me. I'll wait a while to see if anyone else has an opinion, but otherwise it looks good.

@LingMan
Copy link
Contributor Author

LingMan commented Sep 22, 2020

I feel what @Mark-Simulacrum said regarding !matches!(exp, pat). It does blend in, although with the original expanded match blocks true/false and false/true also started to blend fairly quickly while writing this. Possibly because those two words are of similar length and the first case is far more common than the second.

Certainly made me check for something like matches_not!(exp, pat).

@varkor
Copy link
Member

varkor commented Oct 5, 2020

@bors r+ rollup

Thanks!

@bors
Copy link
Contributor

bors commented Oct 5, 2020

📌 Commit a6ff925 has been approved by varkor

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Oct 5, 2020
@joshtriplett
Copy link
Member

I think the appearance of !matches! is one more good argument for a first-class operator like is. But until we have that, !matches!(...) doesn't seem unreasonable.

bors added a commit to rust-lang-ci/rust that referenced this pull request Oct 6, 2020
Rollup of 13 pull requests

Successful merges:

 - rust-lang#76388 (Add a note about the panic behavior of math operations on time objects)
 - rust-lang#76855 (Revamp rustdoc docs about documentation using `cfg`)
 - rust-lang#76995 (Reduce boilerplate with the matches! macro)
 - rust-lang#77228 (Add missing examples for MaybeUninit)
 - rust-lang#77528 (Avoid unchecked casts in net parser)
 - rust-lang#77534 (Disallow overriding forbid in same scope)
 - rust-lang#77555 (Allow anyone to set regression labels)
 - rust-lang#77558 (Rename bootstrap/defaults/{config.toml.PROFILE => config.PROFILE.toml})
 - rust-lang#77559 (Fix rustdoc warnings about invalid Rust syntax)
 - rust-lang#77560 (Fix LitKind's byte buffer to use refcounted slice)
 - rust-lang#77573 (Hint doc use convert::identity relative link)
 - rust-lang#77587 (Fix span for unicode escape suggestion.)
 - rust-lang#77591 (Record `expansion_that_defined` into crate metadata)

Failed merges:

r? `@ghost`
@bors bors merged commit d50349b into rust-lang:master Oct 6, 2020
@rustbot rustbot added this to the 1.49.0 milestone Oct 6, 2020
@LingMan LingMan deleted the middle_matches branch October 6, 2020 13:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-cleanup Category: PRs that clean code up or issues documenting cleanup. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants