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.

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

All uses of extern fn should mean extern "C" fn #12328

Merged
merged 1 commit into from
Feb 24, 2014
Merged

Conversation

nrc
Copy link
Member

@nrc nrc commented Feb 16, 2014

No description provided.

@nrc
Copy link
Member Author

nrc commented Feb 16, 2014

closes #9309

@alexcrichton
Copy link
Member

Could you amend the "Closes #XXXX" into the commit message as well? That way the issue will be automatically closed when it gets merged to master.

-include ../tools.mk

all: $(call STATICLIB,cfoo)
$(RUSTC) foo.rs
Copy link
Member

Choose a reason for hiding this comment

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

This may be better done as a run-pass test instead of a run-make test. It's safe to assume that we link to libc, so you can have extern { fn printf(); } or any symbol defined in libc.

@nrc
Copy link
Member Author

nrc commented Feb 16, 2014

Sorry, missed the inline comment and just saw the one about the commit message. Fixing now...

@nrc
Copy link
Member Author

nrc commented Feb 17, 2014

@alexcrichton ready for re-review

// option. This file may not be copied, modified, or distributed
// except according to those terms.

// Ensure that declarations and types which use `extern fn` both have the same
Copy link
Contributor

Choose a reason for hiding this comment

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

Whitespace in here.

@nrc
Copy link
Member Author

nrc commented Feb 19, 2014

@bors: retry

@alexcrichton
Copy link
Member

Alas, just another case of pub fn main for a run-pass test instead of fn main

@pnkfelix
Copy link
Member

I wonder if the frequency of this problem implies we should lint for it,

@alexcrichton
Copy link
Member

(needs a rebase)

@nrc
Copy link
Member Author

nrc commented Feb 24, 2014

rebased

@brendanzab
Copy link
Member

Why is this? Wouldn't it make more sense to have extern fn mean extern "Rust" fn? This is looking to the future here. We are our own language. (I think I have discussed this with @nikomatsakis in the past)

@brson
Copy link
Contributor

brson commented Feb 24, 2014

@bjz That's an interesting point. Worth considering I think, but as a followup.

@brendanzab
Copy link
Member

@brson yeah, I commented on #9309.

@bors bors closed this Feb 24, 2014
@bors bors merged commit 317a253 into rust-lang:master Feb 24, 2014
bors added a commit to rust-lang-ci/rust that referenced this pull request Jul 25, 2022
matthiaskrgr pushed a commit to matthiaskrgr/rust that referenced this pull request Mar 21, 2024
fix span calculation for non-ascii in `needless_return`

Fixes rust-lang#12491

Probably fixes rust-lang#12328 as well, but that one has no reproducer, so 🤷

The bug was that the lint used `rfind()` for finding the byte index of the start of the previous non-whitespace character:
```
// abc\n     return;
     ^
```
... then subtracting one to get the byte index of the actual whitespace (the `\n` here).
(Subtracting instead of adding because it treats this as the length from the `return` token to the `\n`)

That's correct for ascii, like here, and will get us to the `\n`, however for non ascii, the `c` could be multiple bytes wide, which would put us in the middle of a codepoint if we simply subtract 1 and is what caused the ICE.

There's probably a lot of ways we could fix this.
This PR changes it to iterate backwards using bytes instead of characters, so that when `rposition()` finally finds a non-whitespace byte, we *know* that we've skipped exactly 1 byte. This was *probably*(?) what the code was intending to do

changelog: Fix ICE in [`needless_return`] when previous line end in a non-ascii character
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants