Skip to content

Fixed XFailed test x86stdcall #10363

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

Merged
merged 1 commit into from
Nov 18, 2013
Merged

Conversation

astrieanna
Copy link
Contributor

  • moved extern inside module
  • changed extern "stdcall" to extern "system"
  • changed cfg(target_os="win32") to cfg(windows)
  • only run on Windows && x86, (not x86_64)
  • updated copyright dates

@alexcrichton
Copy link
Member

Could you also rebase this into a single commit? Also feel free to ping this pull request when you update it because sadly github doesn't send out any notifications about pull request force-pushes.

@astrieanna
Copy link
Contributor Author

@alexcrichton Rebased into one commit. :)

#[cfg(target_os = "win32")]
extern "stdcall" mod kernel32 {
#[cfg(windows)]
#[cfg(target_arch = "x86")]
Copy link
Contributor

Choose a reason for hiding this comment

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

#[cfg(target_arch = "x86")] lines are not necessary since extern "system" also covers x86_64. Same for line 24.
In fact, they caused test failure since #[cfg(windows)] #[cfg(target_arch = "x86")] is "or" relation. (you can write #[cfg(windows), target_arch = "x86")] if you want "and" relation.)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks!

@astrieanna
Copy link
Contributor Author

@alexcrichton @klutzy Thanks! I've removed the target_arch lines now.

@huonw
Copy link
Member

huonw commented Nov 17, 2013

I think the log(error, actual) line should become info!("actual = {}", actual).

(The log(error, ...) syntax was the historical form of error!(...) and this test has clearly been xfail'd for long enough to miss the syntax upgrade... and we should be using info! here anyway, as error! prints by default.)

@astrieanna
Copy link
Contributor Author

@huonw thanks! Fixed and rebased.

@astrieanna
Copy link
Contributor Author

I don't know how to test that this runs locally, since I don't have a win32 machine. =/

The test fails because kernel32::SetLastEroor and kernel32::GetLastError are private. I assume that implies that I need to put the pub keyword before each of the those two functions?

@alexcrichton
Copy link
Member

That should do the trick!

There was a syntax error because the `extern "stdcall"` was outside the module instead of inside it.

* moved `extern` inside module
* change `extern "stdcall"` to `extern "system"`
* change `cfg(target_os="win32")` to `cfg(windows)`
* updated copyright dates
* changed log(error, ...) => info!(....)
* added `pub` keyword to kernel32 functions
@astrieanna
Copy link
Contributor Author

@alexcrichton Ok, fixed [again].

bors added a commit that referenced this pull request Nov 18, 2013
* moved `extern` inside module
* changed `extern "stdcall"` to `extern "system"`
* changed `cfg(target_os="win32")` to `cfg(windows)`
* only run on Windows && x86, (not x86_64)
* updated copyright dates
@bors bors closed this Nov 18, 2013
@bors bors merged commit 69768f7 into rust-lang:master Nov 18, 2013
Jarcho pushed a commit to Jarcho/rust that referenced this pull request Feb 26, 2023
[significant_drop_tightening] Ignore inexpensive statements

Not all statements that follow the last use of a lock guard are expensive and can therefore be ignored by the lint.

```rust
pub fn foo() -> i32 {
    let mutex = Mutex::new(1);
    let lock = mutex.lock().unwrap();
    let rslt = *lock;
    let another = rslt;
   another
}
```

---

changelog: [`significant_drop_tightening`]: No longer lints for inexpensive statements after the lock guard
[rust-lang#10363](rust-lang/rust-clippy#10363)
<!-- changelog_checked -->
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.

5 participants