enum size lint by emberian · Pull Request #14300 · rust-lang/rust

emberian · 2014-05-20T00:42:37Z

See commits for details.

lilyball · 2014-05-20T00:51:11Z

src/librustc/middle/trans/base.rs

A full sort is unnecessary. You can just iterate the sizes once and track the two largest values.

let (a,b) = sizes.iter().fold((0, 0), |(a,b), &(_,sz)| if sz > a { (sz,a) } else if sz > b { (a,sz) } else { (a,b) }); if b > 0 && a > b*3 { // ...

Yeah, but that's, like, work!

lilyball · 2014-05-20T00:52:56Z

I want to write a lint that needs information from trans, similar to what you're doing here. Would it be possible to add support for post-trans lints as @huonw suggested in #10362, instead of trying to teach trans special behavior for specific lints?

huonw · 2014-05-20T00:53:36Z

src/librustc/middle/lint.rs

Maybe this could be something like

node_levels: HashMap<(ast::NodeId, Lint), (level, LintSource)>

to allow other trans/post-trans lints naturally. (This could easily be a case of YAGNI though.)

emberian · 2014-05-20T00:59:00Z

@kballard @huonw My first iteration of this PR was a separate lint pass that re-walked the AST, but with the trans context. It ended up being pretty gnarly and with lots of duplication. With the suggested node_levels, which nodes are you going to track, and for which lints? There are over 150000 nodes in a given build of rustc, and you need to decide what you are going to track. Of course, it could be done per-lint... it might not be awful.

huonw · 2014-05-20T01:01:57Z

Yeah, I was thinking only those lints that trans is actually interested in (so ATM it would just be storing (node_id, EnumSizeVariance)).

lilyball · 2014-05-20T01:15:01Z

@cmr I'm interested in having a lint that enforces that a given enum (marked with an attribute) always benefits from the null pointer optimization. And by that I mean it's an enum that takes a type parameter and I want to flag any uses of the enum that result in a concrete type that's not null-pointer-optimized. I would then add this attribute to libc::Nullable, and promote that type as the right type to use for nullable functions in extern blocks. We're currently recommending using Option, but this use only works if we can guarantee that it's null-pointer-optimized, and right now we make no such guarantee.

More specifically, I'd want to ensure that it maps to RawNullablePointer (as opposed to StructWrappedNullablePointer), although that part shouldn't really be in question (as it's a function of the number of fields of the non-nullary variant), but it is part of the guarantee we need to make for correct FFI.

Note that this lint also requires knowing which enums to consider. Implementing this on top of your current hacky work (assuming node_levels instead of enum_levels) could be done by only recording the level in the map if the enum is marked with the attribute, and otherwise ignoring it. That way trans would consider any non-annotated enums to be Allow even though the enum itself would default to either warn or error.

lilyball · 2014-05-20T01:17:34Z

Something to consider is that, if we ever get pluggable lints (which I'd really like to have), then special-casing the specific lints that need post-trans info won't work very well (i.e. the pluggable lints won't be able to be special-cased).

lilyball · 2014-05-20T01:23:04Z

src/librustc/middle/trans/base.rs

You seem to be doing the work of testing every single enum even if the level is allow, and you only check to see if it's actually allow down on line 1587. You should make sure it's not allow right here.

emberian · 2014-05-20T05:33:36Z

@kballard it's true that pluggable lints are going to have a painful life. But, they can always do a separate walk through the AST. This is really just an optimization.

huonw · 2014-05-20T05:41:46Z

src/librustc/middle/lint.rs

s/swith/with/

huonw · 2014-05-20T05:53:33Z

Does this handle generic enums? At the very least, it shouldn't ICE (i.e. testcase please).

emberian · 2014-05-20T06:00:04Z

It runs on all of the current rust source (it was warn by default) runs
with it. I'll add tests.

On Mon, May 19, 2014 at 10:53 PM, Huon Wilson notifications@github.comwrote:

Does this handle generic enums? At the very least, it shouldn't ICE (i.e.
testcase please).

—
Reply to this email directly or view it on GitHubhttps://github.com//pull/14300#issuecomment-43588753
.

http://octayn.net/

emberian · 2014-05-20T06:52:01Z

I believe I've addressed the concerns above, re-review please?

lilyball · 2014-05-20T06:54:32Z

src/librustc/middle/trans/base.rs

The default is allow, so why are you assuming that no entry at all means you should care about the size? You're just going to end up deciding it's allow later and not emitting the lint.

lilyball · 2014-05-20T06:55:40Z

You're still sorting when you could just be folding to find the two largest sizes. I even gave you the code for it.

You're also still inserting an entry in the node map for every enum. That just seems like a waste of memory, when you could only insert non-allow nodes.

emberian · 2014-05-20T06:59:08Z

Ah yes, sorry.

emberian · 2014-05-20T07:36:33Z

@kballard updated

lilyball · 2014-05-20T20:10:57Z

src/librustc/middle/trans/base.rs

You don't need this count. If slargest > 0 then you obviously have at least 2 non-zero sizes.

huonw · 2014-05-21T13:22:35Z

BTW, there's actually already min_max for iterators. (The return value has the .into_option helper too.)

emberian · 2014-05-21T14:39:17Z

I want the index of the largest element, so that doesn't seem super useful?

On Wed, May 21, 2014 at 6:22 AM, Huon Wilson notifications@github.comwrote:

BTW, there's actually already min_max for iteratorshttp://static.rust-lang.org/doc/master/core/iter/trait.OrdIterator.html#tymethod.min_max.
(The return value has the .into_option helper too.)

—
Reply to this email directly or view it on GitHubhttps://github.com//pull/14300#issuecomment-43753052
.

http://octayn.net/

lilyball · 2014-05-21T17:01:43Z

Not just the index, you also want the two largest values, not the largest and smallest.

lilyball reviewed May 20, 2014
View reviewed changes

huonw reviewed May 20, 2014
View reviewed changes

lilyball reviewed May 20, 2014
View reviewed changes

huonw reviewed May 20, 2014
View reviewed changes

src/librustc/middle/lint.rs Outdated

Copy link

Contributor

huonw May 20, 2014

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

s/swith/with/

lilyball reviewed May 20, 2014
View reviewed changes

Uh oh!

Conversation

emberian commented May 20, 2014

Uh oh!

lilyball May 20, 2014

Choose a reason for hiding this comment

Uh oh!

emberian May 20, 2014

Choose a reason for hiding this comment

Uh oh!

lilyball commented May 20, 2014

Uh oh!

huonw May 20, 2014

Choose a reason for hiding this comment

Uh oh!

emberian commented May 20, 2014

Uh oh!

huonw commented May 20, 2014

Uh oh!

lilyball commented May 20, 2014

Uh oh!

lilyball commented May 20, 2014

Uh oh!

lilyball May 20, 2014

Choose a reason for hiding this comment

Uh oh!

emberian commented May 20, 2014

Uh oh!

huonw May 20, 2014

Choose a reason for hiding this comment

Uh oh!

huonw commented May 20, 2014

Uh oh!

emberian commented May 20, 2014

Uh oh!

emberian commented May 20, 2014

Uh oh!

lilyball May 20, 2014

Choose a reason for hiding this comment

Uh oh!

lilyball commented May 20, 2014

Uh oh!

emberian commented May 20, 2014

Uh oh!

emberian commented May 20, 2014

Uh oh!

lilyball May 20, 2014

Choose a reason for hiding this comment

Uh oh!

huonw commented May 21, 2014

Uh oh!

emberian commented May 21, 2014

Uh oh!

lilyball commented May 21, 2014

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants