factor out pluralisation remains after #64280 #64342

glorv · 2019-09-10T10:35:14Z

there are two case that doesn't not match the original macro pattern at here and here as the provided param is already a bool or the check condition is not x != 1, so I change the macro accept a boolean expr instead of number to fit all the cases.

@Centril please review

Fixes #64238.

rust-highfive · 2019-09-10T10:35:17Z

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @estebank (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.

varkor

@glorv: thanks for the pull request! Instead of adding another variant to the macro, I would instead use the original macro and refactor the methods that have a plural: bool. E.g.
https://github.com/rust-lang/rust/blob/5608ea6277e6ab5421318068c47647a289d83d82/src/librustc_lint/unused.rs#L254
could pass len instead of true and then pluralise! could be used as normal.

varkor · 2019-09-10T11:32:39Z

src/tools/compiletest/src/runtest.rs

@@ -36,6 +36,8 @@ use log::*;
 use crate::extract_gdb_version;
 use crate::is_android_gdb_target;

+use syntax::errors::pluralise;


You can't use syntax here: I might just leave this one as is.

varkor · 2019-09-10T11:33:05Z

r? @varkor

Alexendoo · 2019-09-18T19:58:18Z

Ping from triage, any updates? @varkor

src/librustc_errors/lib.rs

varkor · 2019-09-18T20:11:24Z

@glorv: sorry, I didn't notice that you had pushed new changes. I think you want to keep the macro definition as-is: you should just be passing an integer to it. You'll just need to do some refactoring around the locations where there's a boolean value.

src/librustc_lint/unused.rs

glorv · 2019-09-19T04:51:00Z

@varkor @estebank thanks for the review and comments. I change the macro back to the origin format and refactor the effected code, please have another review about that.

varkor · 2019-09-19T22:01:51Z

@glorv: this all looks good to me! Thanks for your patience!

@bors r+ rollup

bors · 2019-09-19T22:01:52Z

📌 Commit 1ab5593 has been approved by varkor

@Centril

factor out pluralisation remains after rust-lang#64280 there are two case that doesn't not match the original macro pattern at [here](https://github.com/rust-lang/rust/blob/master/src/librustc_lint/unused.rs#L146) and [here](https://github.com/rust-lang/rust/blob/master/src/libsyntax/parse/diagnostics.rs#L539) as the provided param is already a bool or the check condition is not `x != 1`, so I change the macro accept a boolean expr instead of number to fit all the cases. @Centril please review Fixes rust-lang#64238.

@ghost

Rollup of 8 pull requests Successful merges: - #64136 (Document From trait for LhsExpr in parser) - #64342 (factor out pluralisation remains after #64280) - #64387 (Fix redundant semicolon lint interaction with proc macro attributes) - #64498 (When possible point at argument causing item obligation failure) - #64615 (rustbuild: Turn down compression on exe installers) - #64617 (rustbuild: Turn down compression on msi installers) - #64618 (rustbuild: Improve output of `dist` step) - #64621 (Add Compatibility Notes to RELEASES.md for 1.38.0) Failed merges: r? @ghost

@Centril

factor out pluralisation remains after rust-lang#64280 there are two case that doesn't not match the original macro pattern at [here](https://github.com/rust-lang/rust/blob/master/src/librustc_lint/unused.rs#L146) and [here](https://github.com/rust-lang/rust/blob/master/src/libsyntax/parse/diagnostics.rs#L539) as the provided param is already a bool or the check condition is not `x != 1`, so I change the macro accept a boolean expr instead of number to fit all the cases. @Centril please review Fixes rust-lang#64238.

@Centril

factor out pluralisation remains after rust-lang#64280 there are two case that doesn't not match the original macro pattern at [here](https://github.com/rust-lang/rust/blob/master/src/librustc_lint/unused.rs#L146) and [here](https://github.com/rust-lang/rust/blob/master/src/libsyntax/parse/diagnostics.rs#L539) as the provided param is already a bool or the check condition is not `x != 1`, so I change the macro accept a boolean expr instead of number to fit all the cases. @Centril please review Fixes rust-lang#64238.

@Centril

factor out pluralisation remains after rust-lang#64280 there are two case that doesn't not match the original macro pattern at [here](https://github.com/rust-lang/rust/blob/master/src/librustc_lint/unused.rs#L146) and [here](https://github.com/rust-lang/rust/blob/master/src/libsyntax/parse/diagnostics.rs#L539) as the provided param is already a bool or the check condition is not `x != 1`, so I change the macro accept a boolean expr instead of number to fit all the cases. @Centril please review Fixes rust-lang#64238.

@Centril

factor out pluralisation remains after rust-lang#64280 there are two case that doesn't not match the original macro pattern at [here](https://github.com/rust-lang/rust/blob/master/src/librustc_lint/unused.rs#L146) and [here](https://github.com/rust-lang/rust/blob/master/src/libsyntax/parse/diagnostics.rs#L539) as the provided param is already a bool or the check condition is not `x != 1`, so I change the macro accept a boolean expr instead of number to fit all the cases. @Centril please review Fixes rust-lang#64238.

@Centril

factor out pluralisation remains after rust-lang#64280 there are two case that doesn't not match the original macro pattern at [here](https://github.com/rust-lang/rust/blob/master/src/librustc_lint/unused.rs#L146) and [here](https://github.com/rust-lang/rust/blob/master/src/libsyntax/parse/diagnostics.rs#L539) as the provided param is already a bool or the check condition is not `x != 1`, so I change the macro accept a boolean expr instead of number to fit all the cases. @Centril please review Fixes rust-lang#64238.

@Centril

factor out pluralisation remains after rust-lang#64280 there are two case that doesn't not match the original macro pattern at [here](https://github.com/rust-lang/rust/blob/master/src/librustc_lint/unused.rs#L146) and [here](https://github.com/rust-lang/rust/blob/master/src/libsyntax/parse/diagnostics.rs#L539) as the provided param is already a bool or the check condition is not `x != 1`, so I change the macro accept a boolean expr instead of number to fit all the cases. @Centril please review Fixes rust-lang#64238.

@Centril

factor out pluralisation remains after rust-lang#64280 there are two case that doesn't not match the original macro pattern at [here](https://github.com/rust-lang/rust/blob/master/src/librustc_lint/unused.rs#L146) and [here](https://github.com/rust-lang/rust/blob/master/src/libsyntax/parse/diagnostics.rs#L539) as the provided param is already a bool or the check condition is not `x != 1`, so I change the macro accept a boolean expr instead of number to fit all the cases. @Centril please review Fixes rust-lang#64238.

@Centril

factor out pluralisation remains after rust-lang#64280 there are two case that doesn't not match the original macro pattern at [here](https://github.com/rust-lang/rust/blob/master/src/librustc_lint/unused.rs#L146) and [here](https://github.com/rust-lang/rust/blob/master/src/libsyntax/parse/diagnostics.rs#L539) as the provided param is already a bool or the check condition is not `x != 1`, so I change the macro accept a boolean expr instead of number to fit all the cases. @Centril please review Fixes rust-lang#64238.

@ghost

Rollup of 9 pull requests Successful merges: - #64010 (Stabilize `param_attrs` in Rust 1.39.0) - #64136 (Document From trait for LhsExpr in parser) - #64342 (factor out pluralisation remains after #64280) - #64347 (Add long error explanation for E0312) - #64621 (Add Compatibility Notes to RELEASES.md for 1.38.0) - #64632 (remove the extra comma after the match arm) - #64640 (No home directory on vxWorks) - #64641 (Exempt extern "Rust" from improper_ctypes) - #64642 (Fix the span used to suggest avoiding for-loop moves) Failed merges: r? @ghost

rust-highfive assigned estebank Sep 10, 2019

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Sep 10, 2019

glorv mentioned this pull request Sep 10, 2019

Factor out pluralisation checks in diagnostics #64238

Closed

varkor suggested changes Sep 10, 2019

View reviewed changes

rust-highfive assigned varkor and unassigned estebank Sep 10, 2019

This comment has been minimized.

Sign in to view

glorv force-pushed the master branch from f8b286f to c9fcb5c Compare September 11, 2019 01:19

This comment has been minimized.

Sign in to view

glorv force-pushed the master branch 3 times, most recently from 18b7ef5 to 966fe50 Compare September 11, 2019 02:59

varkor reviewed Sep 18, 2019

View reviewed changes

src/librustc_errors/lib.rs Outdated Show resolved Hide resolved

estebank reviewed Sep 18, 2019

View reviewed changes

src/librustc_lint/unused.rs Show resolved Hide resolved

glorv force-pushed the master branch from 966fe50 to 6246f25 Compare September 19, 2019 04:48

This comment has been minimized.

Sign in to view

glorv force-pushed the master branch 5 times, most recently from 70c695a to 20dc4e0 Compare September 19, 2019 07:13

This comment has been minimized.

Sign in to view

glorv force-pushed the master branch 2 times, most recently from 7dd1570 to 93d8eb0 Compare September 19, 2019 16:02

This comment has been minimized.

Sign in to view

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 Sep 19, 2019

Centril mentioned this pull request Sep 20, 2019

Rollup of 8 pull requests #64626

Closed

Centril mentioned this pull request Sep 20, 2019

Rollup of 9 pull requests #64637

Closed

tmandry mentioned this pull request Sep 20, 2019

Rollup of 10 pull requests #64643

Closed

tmandry mentioned this pull request Sep 20, 2019

Rollup of 10 pull requests #64644

Closed

tmandry mentioned this pull request Sep 20, 2019

Rollup of 9 pull requests #64645

Closed

tmandry mentioned this pull request Sep 20, 2019

Rollup of 10 pull requests #64646

Closed

tmandry mentioned this pull request Sep 21, 2019

Rollup of 10 pull requests #64647

Closed

Centril mentioned this pull request Sep 21, 2019

Rollup of 7 pull requests #64657

Closed

Centril mentioned this pull request Sep 21, 2019

Rollup of 9 pull requests #64658

Merged

bors merged commit 1ab5593 into rust-lang:master Sep 21, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

factor out pluralisation remains after #64280 #64342

factor out pluralisation remains after #64280 #64342

glorv commented Sep 10, 2019 •

edited

Loading

rust-highfive commented Sep 10, 2019

varkor left a comment

varkor Sep 10, 2019

varkor commented Sep 10, 2019

This comment has been minimized.

This comment has been minimized.

Alexendoo commented Sep 18, 2019

varkor commented Sep 18, 2019

glorv commented Sep 19, 2019

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

varkor commented Sep 19, 2019

bors commented Sep 19, 2019

factor out pluralisation remains after #64280 #64342

factor out pluralisation remains after #64280 #64342

Conversation

glorv commented Sep 10, 2019 • edited Loading

rust-highfive commented Sep 10, 2019

varkor left a comment

Choose a reason for hiding this comment

varkor Sep 10, 2019

Choose a reason for hiding this comment

varkor commented Sep 10, 2019

This comment has been minimized.

This comment has been minimized.

Alexendoo commented Sep 18, 2019

varkor commented Sep 18, 2019

glorv commented Sep 19, 2019

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

varkor commented Sep 19, 2019

bors commented Sep 19, 2019

glorv commented Sep 10, 2019 •

edited

Loading