-
Notifications
You must be signed in to change notification settings - Fork 762
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
[rust 2021] migrate to rust 2021 #1159
Conversation
Signed-off-by: Chojan Shang <psiace@outlook.com>
Thanks for the contribution! Please review the labels and make any necessary changes. |
@@ -87,7 +87,10 @@ impl DataBlock { | |||
.columns() | |||
.iter() | |||
.zip(rhs.columns().iter()) | |||
.map(|(a, b)| DataColumnCommon::merge_columns(a, b, indices)) | |||
.map(|(a, b)| { | |||
let _ = &indices; |
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.
This line used for?
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.
This line used for?
see https://doc.rust-lang.org/nightly/edition-guide/rust-2021/disjoint-capture-in-closures.html
Auto gen by cargo fix
. This is a conservative analysis: in many cases, these dummy lets can be safely removed and your program will work fine.
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 doesn't look like a good style : (
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.
Maybe we can do it with macros? similar to '[&captures_var] (args) {} in C++`.
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.
Maybe we can do it with macros? similar to '[&captures_var] (args) {} in C++`.
We can try to remove these dummy let manually. If it is really necessary, consider other options.
.map(|opt_idx| opt_idx.map(|idx| *array_values.get_unchecked(idx))); | ||
let iter = indices.into_iter().map(|opt_idx| { | ||
opt_idx.map(|idx| { | ||
let _ = &array_values; |
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.
Why rust 2021 turns to be more complicated.
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.
😭
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.
Why rust 2021 turns to be more complicated.
I don't think it becomes more complicated.
This is a conservative analysis: in many cases, these dummy lets can be safely removed and your program will work fine.
Starting in Rust 2021, closures captures are more precise. Typically they will only capture the fields they use (in some cases, they might capture more than just what they use, see the Rust reference for full details).
We can try to adjust these codes later. Just announcing that we are moving towards Rust 2021. |
Since the Rust official says:
We'd better make this PR commit to an independent branch likes |
Okay, thanks. |
Codecov Report
@@ Coverage Diff @@
## master #1159 +/- ##
=========================================
- Coverage 71% 59% -12%
=========================================
Files 446 6 -440
Lines 26330 96 -26234
=========================================
- Hits 18714 57 -18657
+ Misses 7616 39 -7577 Continue to review full report at Codecov.
|
If there are no major problems, merging to the maater branch is ok, we also want to help Rust 2021 mature faster, not simply testing, and Rust 2021 nightly will be released soon. |
There seems to be no problem, I will check if there is some room for improvement, and then merge before next Wednesday. |
Signed-off-by: Chojan Shang <psiace@outlook.com>
Signed-off-by: Chojan Shang <psiace@outlook.com>
Signed-off-by: Chojan Shang <psiace@outlook.com>
Signed-off-by: Chojan Shang <psiace@outlook.com>
Signed-off-by: Chojan Shang <psiace@outlook.com>
Hello @PsiACE, 🎉 Thank you for opening the pull request! 🎉 |
Finally, I removed all useless dummy lets. I think we can announce a successful migration to Rust 2021. The only regret is that the |
Great, waiting for rust-lang/rust#87468 to merge. |
87468 has merged, we could start this task again. |
Yes, but we still need to wait for the new nightly version. This PR may be completed later today. |
Signed-off-by: Chojan Shang <psiace@outlook.com>
Signed-off-by: Chojan Shang <psiace@outlook.com>
@BohuTANG Done. Once merged, we will enter the era of Rust 2021. |
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.
Looks great to me!
CI Passed |
Signed-off-by: Chojan Shang psiace@outlook.com
I hereby agree to the terms of the CLA available at: https://datafuse.rs/policies/cla/
Summary
Migrate to rust 2021
cargo fmt fails with error "Invalid value for `--edition`", see:
--edition
" rust-lang/rust#87363wait:
Changelog
Related Issues
Fixes #1149
Test Plan
Unit Tests
Stateless Tests