large_enum_variants lint suggests to box variants above a configurable limit #1492

oli-obk · 2017-01-30T11:33:24Z

fixes #1127
obsoletes #1187

…e limit

mcarton · 2017-01-30T18:38:10Z

clippy_lints/src/large_enum_variant.rs

+//! lint when there are large variants on an enum
+
+use rustc::lint::*;
+use rustc::hir::*;


Well, fortunately you did not promise 😄

in my defense, I only rebased this code ;)

In my defense, this does not appear in either the commit info (“@foo committed with @bar”), the first commit message, the blame info (you don't seem to have used git rebase?), or the PR's comment (“obsoletes #xyz” implies this is more than just a rebase).

mcarton · 2017-01-30T18:38:14Z

clippy_lints/src/large_enum_variant.rs

+use rustc::ty::TypeFoldable;
+use rustc::traits::Reveal;
+
+/// **What it does:** Checks for large variants on enums.


I prefer “enumeration” or “enums”.

mcarton · 2017-01-30T18:43:48Z

clippy_lints/src/large_enum_variant.rs

+                                           LARGE_ENUM_VARIANT,
+                                           def.variants[i].span,
+                                           &format!("large enum variant found on variant `{}`", variant.name),
+                                           "consider boxing the large branches to reduce the total size of the enum");


Make a suggestion if there is only one field?

s/branch/field

mcarton · 2017-01-30T18:44:39Z

tests/compile-fail/large_enum_variant.rs

+}
+
+enum LargeEnumGeneric<A: SomeTrait> {
+    Var(A::Item), // regression test, this used to ICE


You add regression tests even before the PR is merged? 😄

I got tired of fixing multiple commits during rebasing and simply merged them into one

Arnavion · 2017-02-04T22:43:52Z

Needs to be documented at https://github.com/Manishearth/rust-clippy/wiki#large_enum_variant

Arnavion · 2017-02-04T22:57:55Z

Also, this lint warns on all variants that exceed 200 bytes in size, as opposed to if the difference between the smallest and largest variant exceeds 200 bytes. Is that intentional? The latter seems to make more sense to me.

Edit: From #rust:

<Arnavion> So, clippy add the (as yet undocumented) lint recently that warns if an enum variant is more than 200 bytes and suggests boxing the fields of the variant. The justification is "Enum size is bounded by the largest variant. Having a large variant can penalize the memory layout of that enum."
<Arnavion> Is that really a valid reason? Or should the lint be warning if the *difference* between the smallest and largest fields is more than 200 bytes?
<rkruppe-phone> Arnavion: yeah relative sizes between variants sounds more correct (I wouldn't use 200 bytes as cut off though)

Edit: This was fixed in https://github.com/Manishearth/rust-clippy/pull/1512

Manishearth · 2017-02-04T23:29:39Z

Updated the wiki.

large_enum_variants lint suggests to box variants above a configurabl…

82dd50d

…e limit

oli-obk force-pushed the largeEnumVariant branch from 4cf08ab to 82dd50d Compare January 30, 2017 12:18

mcarton suggested changes Jan 30, 2017

View reviewed changes

oli-obk added 3 commits January 31, 2017 08:36

improve messages and add suggestions

9bda699

rustfmt

75f605c

place the error checks on the correct lines

12eeffd

mcarton merged commit b1be0d6 into master Jan 31, 2017

mcarton deleted the largeEnumVariant branch January 31, 2017 18:12

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

large_enum_variants lint suggests to box variants above a configurable limit #1492

large_enum_variants lint suggests to box variants above a configurable limit #1492

oli-obk commented Jan 30, 2017

mcarton Jan 30, 2017

oli-obk Jan 31, 2017

mcarton Jan 31, 2017 •

edited

Loading

mcarton Jan 30, 2017

mcarton Jan 30, 2017

mcarton Jan 30, 2017

oli-obk Jan 31, 2017

Arnavion commented Feb 4, 2017

Arnavion commented Feb 4, 2017 •

edited

Loading

Manishearth commented Feb 4, 2017

large_enum_variants lint suggests to box variants above a configurable limit #1492

large_enum_variants lint suggests to box variants above a configurable limit #1492

Conversation

oli-obk commented Jan 30, 2017

mcarton Jan 30, 2017

Choose a reason for hiding this comment

oli-obk Jan 31, 2017

Choose a reason for hiding this comment

mcarton Jan 31, 2017 • edited Loading

Choose a reason for hiding this comment

mcarton Jan 30, 2017

Choose a reason for hiding this comment

mcarton Jan 30, 2017

Choose a reason for hiding this comment

mcarton Jan 30, 2017

Choose a reason for hiding this comment

oli-obk Jan 31, 2017

Choose a reason for hiding this comment

Arnavion commented Feb 4, 2017

Arnavion commented Feb 4, 2017 • edited Loading

Manishearth commented Feb 4, 2017

mcarton Jan 31, 2017 •

edited

Loading

Arnavion commented Feb 4, 2017 •

edited

Loading