-
Notifications
You must be signed in to change notification settings - Fork 1.7k
Fixed issue #15192 #15241
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
base: master
Are you sure you want to change the base?
Fixed issue #15192 #15241
Conversation
Lintcheck changes for 0ef10a6
This comment will be updated if you push new changes |
The
(note also the quotes around This way it will be easily transformed into a link to the lint description when integrated in |
@samueltardieu Thanks ! I've updated the changelog line as suggested . |
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.
Hmm, I personally feel like the help message for what we're expecting the user to do to resolve the lint (introduce indirection for example by boxing) was still helpful as a hint.
Even in #![no_std]
crates, one can still use extern crate alloc
and use alloc::boxed::Box<_>
that way (and I'd expect in practice this suggestion is very often still applicable), and even if the user doesn't have the alloc crate, I'd expect there would still be some other moral equivalent to introduce indirection
What do you think about instead of omitting this entirely in #![no_std]
crates, actually "downgrade" the suggestion to just the help message, so it still hints to the user what they can do about it. We could also slightly change the message in case the user really can't use Box<_>
. I'm thinking of something like:
help: consider boxing the large fields or introducing indirection in some other way to reduce the total size of the enum
@@ -0,0 +1,16 @@ | |||
#![no_std] | |||
#![no_main] |
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.
Tests are built as library crates, so you should be able to largely simplify this. The #![no_main]
and panic handling code shouldn't be needed.
#![no_std]
#![warn(clippy::large_enum_variant)]
enum Myenum {
//~^ ERROR: large size difference between variants
Small(u8),
Large([u8; 1024]),
}
@y21 Thanks for the feedback . You are correct that a downgraded help message should be used . I will update my pull request to follow the same and also I'll edit out the |
@y21 I have implemented the suggestions you made . Please review it now. |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
ca8e2a3
to
2614a31
Compare
Requested reviewer is already assigned to this pull request. Please choose another assignee. |
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.
Can you also squash the commits into one?
@@ -149,7 +150,9 @@ impl<'tcx> LateLintPass<'tcx> for LargeEnumVariant { | |||
return; | |||
} | |||
} | |||
diag.span_help(def.variants[variants_size[0].ind].span, help_text); | |||
if !is_no_std_crate(cx) { |
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.
Is this if
here on purpose? My suggestion was to always at least show the help message, no matter what, as a guidance in case the user has some other way to add indirection, even without access to alloc
.
As is implemented right now, it will still suppress the help message entirely in #![no_std]
(as can be seen in the test)
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.
Actually, looking at the git history on this branch, it looks like this was correct in lkshayb@0ac806d , but was then later reverted again in lkshayb@3e6d575
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.
Thx @y21 for pointing this out . I've updated the pr now . and also squashed all the commits into one.
Fixed issue by adding changing the help_text
@y21 I've implemented the changes and squashed all the commits into one , please review now . |
Fixes #15192 by adding checks for no_std while giving out Box recommendation