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

Add _post methods for blocks and crates #31562

Merged
merged 3 commits into from Feb 13, 2016

Conversation

Projects
None yet
7 participants
@llogiq
Contributor

llogiq commented Feb 11, 2016

This fixes #31512 for me.

A bit of explanation: I want to have check_block_post(&mut self, &Context, &Block) and check_crate_post(&mut self, &Context, &Crate) methods in both early and late lint passes. Ideally we'd have _post methods for all operations that walk, but this'll do for now.

@Manishearth r?

@rust-highfive

This comment has been minimized.

Show comment
Hide comment
@rust-highfive

rust-highfive Feb 11, 2016

Collaborator

r? @nikomatsakis

(rust_highfive has picked a reviewer for you, use r? to override)

Collaborator

rust-highfive commented Feb 11, 2016

r? @nikomatsakis

(rust_highfive has picked a reviewer for you, use r? to override)

@llogiq

This comment has been minimized.

Show comment
Hide comment
@llogiq

llogiq Feb 11, 2016

Contributor

r? @Manishearth – I always forget which way 'round...

Contributor

llogiq commented Feb 11, 2016

r? @Manishearth – I always forget which way 'round...

@Manishearth

This comment has been minimized.

Show comment
Hide comment
@Manishearth

Manishearth Feb 11, 2016

Member

Could we have one for items too? That sounds like another logical thing to have a post-check for.

Member

Manishearth commented Feb 11, 2016

Could we have one for items too? That sounds like another logical thing to have a post-check for.

@llogiq

This comment has been minimized.

Show comment
Hide comment
@llogiq

llogiq Feb 11, 2016

Contributor

Sure. I've got the commit here, but my notebook network is flaky. Will commit soon.

Contributor

llogiq commented Feb 11, 2016

Sure. I've got the commit here, but my notebook network is flaky. Will commit soon.

@llogiq

This comment has been minimized.

Show comment
Hide comment
@llogiq

llogiq Feb 11, 2016

Contributor

@Manishearth done.

Contributor

llogiq commented Feb 11, 2016

@Manishearth done.

@Manishearth

This comment has been minimized.

Show comment
Hide comment
@Manishearth

Manishearth Feb 11, 2016

Member

r=me, but also asking @eddyb for a second opinion

Member

Manishearth commented Feb 11, 2016

r=me, but also asking @eddyb for a second opinion

@eddyb

This comment has been minimized.

Show comment
Hide comment
@eddyb

eddyb Feb 11, 2016

Member

I think we should try to experiment with a way to avoid combinatoric explosion, perhaps trait Check<T> with e.g. impl Check<Expr> for MyLint, but I don't see how this could be done cleanly without specialization (same goes for our visitors and folders btw).

Member

eddyb commented Feb 11, 2016

I think we should try to experiment with a way to avoid combinatoric explosion, perhaps trait Check<T> with e.g. impl Check<Expr> for MyLint, but I don't see how this could be done cleanly without specialization (same goes for our visitors and folders btw).

@Manishearth

This comment has been minimized.

Show comment
Hide comment
@Manishearth

Manishearth Feb 11, 2016

Member

That's an interesting idea, but I don't think combinatoric explosion can happen here, or that such changes need to be made here often.

An eventual new design for lint passes would be nice though. Not sure if we need it right now.

Member

Manishearth commented Feb 11, 2016

That's an interesting idea, but I don't think combinatoric explosion can happen here, or that such changes need to be made here often.

An eventual new design for lint passes would be nice though. Not sure if we need it right now.

@Manishearth

This comment has been minimized.

Show comment
Hide comment
Member

Manishearth commented Feb 11, 2016

@bors r+

@bors

This comment has been minimized.

Show comment
Hide comment
@bors

bors Feb 11, 2016

Contributor

📌 Commit d483a6e has been approved by Manishearth

Contributor

bors commented Feb 11, 2016

📌 Commit d483a6e has been approved by Manishearth

@llogiq

This comment has been minimized.

Show comment
Hide comment
@llogiq

llogiq Feb 11, 2016

Contributor

FWIW, I don't think we get combinatoric explosion here. Worst (or best, depending on how you look at it) case we have an entry/exit (or check/post in our current nomenclature) for each node that contains other nodes.

Contributor

llogiq commented Feb 11, 2016

FWIW, I don't think we get combinatoric explosion here. Worst (or best, depending on how you look at it) case we have an entry/exit (or check/post in our current nomenclature) for each node that contains other nodes.

@llogiq

This comment has been minimized.

Show comment
Hide comment
@llogiq

llogiq Feb 12, 2016

Contributor

There is some duplicate error messages in the compiletest output. I doubt it has something to do with the new methods, as there is nothing that uses them yet.

@bors retry

Contributor

llogiq commented Feb 12, 2016

There is some duplicate error messages in the compiletest output. I doubt it has something to do with the new methods, as there is nothing that uses them yet.

@bors retry

Show outdated Hide outdated src/librustc/lint/context.rs
@@ -918,6 +920,7 @@ impl<'a, 'v> ast_visit::Visitor<'v> for EarlyContext<'a> {
run_lints!(cx, check_item, early_passes, it);
cx.visit_ids(|v| v.visit_item(it));
ast_visit::walk_item(cx, it);
run_lints!(cx, check_item, early_passes, it);

This comment has been minimized.

@oli-obk

oli-obk Feb 12, 2016

Contributor

this should be check_item_post

@oli-obk

oli-obk Feb 12, 2016

Contributor

this should be check_item_post

@llogiq

This comment has been minimized.

Show comment
Hide comment
@llogiq

llogiq Feb 12, 2016

Contributor

Thanks, @oli-obk ! Good catch.

Contributor

llogiq commented Feb 12, 2016

Thanks, @oli-obk ! Good catch.

@Manishearth

This comment has been minimized.

Show comment
Hide comment
Member

Manishearth commented Feb 12, 2016

@bors r+

@bors

This comment has been minimized.

Show comment
Hide comment
@bors

bors Feb 12, 2016

Contributor

📌 Commit a270b7b has been approved by Manishearth

Contributor

bors commented Feb 12, 2016

📌 Commit a270b7b has been approved by Manishearth

@bors

This comment has been minimized.

Show comment
Hide comment
@bors

bors Feb 13, 2016

Contributor

⌛️ Testing commit a270b7b with merge 5801991...

Contributor

bors commented Feb 13, 2016

⌛️ Testing commit a270b7b with merge 5801991...

bors added a commit that referenced this pull request Feb 13, 2016

Auto merge of #31562 - llogiq:lint_post, r=Manishearth
This fixes #31512 for me.

A bit of explanation: I want to have `check_block_post(&mut self, &Context, &Block)` and `check_crate_post(&mut self, &Context, &Crate)` methods in both early and late lint passes. Ideally we'd have _post methods for all operations that walk, but this'll do for now.

@Manishearth r?

@bors bors merged commit a270b7b into rust-lang:master Feb 13, 2016

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
homu Test successful
Details

@llogiq llogiq deleted the llogiq:lint_post branch Feb 13, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment