Skip to content

Fix trans for foreign methods#22538

Merged
alexcrichton merged 2 commits into
rust-lang:masterfrom
nagisa:properise-trans-asserts
Feb 23, 2015
Merged

Fix trans for foreign methods#22538
alexcrichton merged 2 commits into
rust-lang:masterfrom
nagisa:properise-trans-asserts

Conversation

@nagisa

@nagisa nagisa commented Feb 19, 2015

Copy link
Copy Markdown
Member

No description provided.

@rust-highfive

rust-highfive commented Feb 19, 2015

Copy link
Copy Markdown
Contributor

r? @nikomatsakis

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

nikomatsakis
nikomatsakis reviewed Feb 19, 2015
View reviewed changes
Comment thread src/librustc_trans/trans/base.rs Outdated

@nikomatsakis nikomatsakis Feb 19, 2015

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This should be a span_bug, not span_fatal, unless you think this is user error (the assert suggests it's a bug in the compiler).

Also, it's always nice to print out the value of f.abi too.

@nikomatsakis

nikomatsakis commented Feb 19, 2015

Copy link
Copy Markdown
Contributor

r+ if the fatal is changed to a bug (ping me or someone else once that's done...)

@nagisa

nagisa commented Feb 19, 2015

Copy link
Copy Markdown
Member Author

note to self:

nmatsakis: it seems like we should report an error in librustc_typeck::astconv::ty_of_method

@nagisa nagisa force-pushed the properise-trans-asserts branch 2 times, most recently from abd4ffd to 0debe6d Compare February 19, 2015 22:01
@nagisa

nagisa commented Feb 19, 2015

Copy link
Copy Markdown
Member Author

I updated the code to report error in typeck, and indeed, it is much nicer.

I’m not happy about the span, though. Currently it spans the whole function and I’d like to only span the extern( ".*")? bit, but I can’t see extern fn being represented in AST, so I conclude it is not easily doable.

@nagisa nagisa closed this Feb 19, 2015
@nagisa nagisa reopened this Feb 19, 2015
@nagisa nagisa force-pushed the properise-trans-asserts branch 5 times, most recently from 70c16e4 to 11e7b20 Compare February 20, 2015 08:00
@nagisa nagisa changed the title Print proper error message on unexpected CC Fix trans for foreign methods Feb 20, 2015
@nagisa nagisa force-pushed the properise-trans-asserts branch from 11e7b20 to ba4cdca Compare February 20, 2015 08:05
@nagisa
Convert some panics to bug diagnostics in trans
0c25e6f
@nagisa
Properly translate methods with foreign CC
9be8ec8 
This fixes a general issue of trying to define extern functions inside impl blocks resulting in
ICE.

Fixes rust-lang#21238
Fixes rust-lang#20734
Fixes rust-lang#19047
@nagisa nagisa force-pushed the properise-trans-asserts branch from ba4cdca to 9be8ec8 Compare February 22, 2015 14:04
@nagisa

nagisa commented Feb 22, 2015

Copy link
Copy Markdown
Member Author

Rebased.

@nikomatsakis

nikomatsakis commented Feb 22, 2015

Copy link
Copy Markdown
Contributor

@bors r+ 9be8ec8

@Manishearth

Manishearth commented Feb 23, 2015

Copy link
Copy Markdown
Member

Needs Manishearth@77b4bba to work on master.

You're part of the rollup, so you shouldn't have to worry about this though.

@Manishearth

Manishearth commented Feb 23, 2015

Copy link
Copy Markdown
Member

Also 315f200 (Windows specific stuff)

Manishearth added a commit to Manishearth/rust that referenced this pull request Feb 23, 2015
@Manishearth @alexcrichton
Rollup merge of rust-lang#22538 - nagisa:properise-trans-asserts, r=n…
bf7cde4 
…ikomatsakis
Manishearth added a commit to Manishearth/rust that referenced this pull request Feb 23, 2015
@Manishearth @alexcrichton
Add marker trait to test (fixup rust-lang#22538)
f17a522
Manishearth added a commit to Manishearth/rust that referenced this pull request Feb 23, 2015
@Manishearth @alexcrichton
Fix windows failure (fixup rust-lang#22538)
1beaebb
@alexcrichton alexcrichton merged commit 9be8ec8 into rust-lang:master Feb 23, 2015
@Aatch

Aatch commented Jun 16, 2015

Copy link
Copy Markdown
Contributor

Is it possible to get this reverted? Or more accurately, changed to completely disallow non-Rust ABI methods? There's a bunch of bugs related to this and I'm not sure non-Rust ABI methods really make sense.

All this PR did was allow them to be declared, but didn't handle the bodies. It also never addressed methods on trait objects.

lnicola added a commit to lnicola/rust that referenced this pull request Jun 8, 2026
@lnicola
Merge pull request rust-lang#22538 from lnicola/bump-actions-gh-pages
cea07bd 
minor: Bump `peaceiris/actions-gh-pages`
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

@nikomatsakis nikomatsakis

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

@nagisa @rust-highfive @nikomatsakis @Manishearth @Aatch @alexcrichton