Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Lint: warn when mixing #[repr(C)] with Drop #24935

Merged
merged 2 commits into from
Apr 30, 2015
Merged

Lint: warn when mixing #[repr(C)] with Drop #24935

merged 2 commits into from
Apr 30, 2015

Conversation

pnkfelix
Copy link
Member

Lint: warn when mixing #[repr(C)] with Drop

Fix #24585

@rust-highfive
Copy link
Collaborator

r? @alexcrichton

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

huonw
huonw reviewed Apr 29, 2015
View reviewed changes
src/librustc_lint/builtin.rs
codemap::DUMMY_SP);
ctx.span_lint(DROP_WITH_REPR_EXTERN,
drop_impl_span,
"Structs with `#[repr(C)]` attribute that \
Copy link
Member

Choose a reason for hiding this comment

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

I believe we generally start compiler messages with a lower case.

Copy link
Member

Choose a reason for hiding this comment

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

I wonder if this message could also be rephrased slightly, maybe:

implementing Drop adds hidden state to a struct, possibly conflicting with the #[repr(C)] attribute

Copy link
Member Author

Choose a reason for hiding this comment

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

I didn't like the fact that I was referring solely to "structs" anyway, when this is slightly more general than that. So, that's fixed now. :)

@alexcrichton
Copy link
Member

Just a few minor nits, otherwise r=me though. Thanks @pnkfelix!

pnkfelix added 2 commits April 29, 2015 23:05
@pnkfelix
lint for mixing #[repr(C)] with an impl of Drop.
78130a4
@pnkfelix
tests for lint that warns about mixing #[repr(C)] with Drop.
2e23d81 
THis includes tests for struct and enum. (I suspect the closure case
is actually unreachable, but i see no harm in including it.)
@pnkfelix
Copy link
Member Author

@bors r=alexcrichton 2e23d81 rollup

alexcrichton added a commit to alexcrichton/rust that referenced this pull request Apr 29, 2015
@alexcrichton
rollup merge of rust-lang#24935: pnkfelix/lint-repr-c-drop
373d620 
Lint: warn when mixing `#[repr(C)]` with Drop

Fix rust-lang#24585
@bors bors merged commit 2e23d81 into rust-lang:master Apr 30, 2015
@pnkfelix
Copy link
Member Author

triage: beta-nominated

@rust-highfive rust-highfive added the beta-nominated Nominated for backporting to the compiler in the beta channel. label Apr 30, 2015
@pnkfelix
Copy link
Member Author

accepted for backport to beta channel.

(this is special case for 1.0; post 1.0 a change like this might need stronger motivation...)

@pnkfelix pnkfelix added the beta-accepted Accepted for backporting to the compiler in the beta channel. label Apr 30, 2015
@alexcrichton alexcrichton mentioned this pull request Apr 30, 2015
Merged
@brson brson removed the beta-nominated Nominated for backporting to the compiler in the beta channel. label Apr 30, 2015
@brson
Copy link
Contributor

brson commented Apr 30, 2015

Backported.

@alexcrichton alexcrichton mentioned this pull request May 1, 2015
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@alexcrichton alexcrichton

Labels
beta-accepted Accepted for backporting to the compiler in the beta channel.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

The Drop flag is a footgun for #[repr(C)]
6 participants
@pnkfelix @rust-highfive @alexcrichton @brson @huonw @bors