-
Notifications
You must be signed in to change notification settings - Fork 13.4k
[MIR] Calling extern functions. #30517
New issue
Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? # to your account
Conversation
r? @pnkfelix (rust_highfive has picked a reviewer for you, use r? to override) |
c50d54c
to
b990190
Compare
@@ -88,6 +88,19 @@ fn test8() -> isize { | |||
Two::two() | |||
} | |||
|
|||
#[link(name = "c")] | |||
extern { | |||
fn printf(format: *const u8, ...) -> i32; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hmm, is there something other than printf
that we could test here?
I mean, I guess its fine (if a little opaque) to rely on the return value of printf
as the basis for the test; but then again, that's relying on them never returning an error code ... seems like there are more direct options here, like strlen
, or qsort
, (well qsort
might be a bit of a stretch).
If you are deliberately trying to test the handling of varargs functions, then maybe go with sscanf
? Basically I'm trying to avoid doing IO where the output isn't actually checked for correctness, since that might mask bugs...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Probably the most obvious VA_ARG function to test is syscall
, but it is system-dependent (duh).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Other functions are fcntl
, ioctl
, execl{,e,p}
. Exec might be a good one I think.
@luqmana sorry it took me so long to get to this r=me if the test is revised or if someone puts up a convincing argument that it isn't worth revising. :) |
Note, #30481 is already in queue and this gonna need significant changes to get rebased. |
☔ The latest upstream changes (presumably #30692) made this pull request unmergeable. Please resolve the merge conflicts. |
Closed in favour of #30845. |
Supersedes #30517 Fixes #29575 cc @luqmana r? @nikomatsakis
Supersedes #30517 Fixes #29575 cc @luqmana r? @nikomatsakis
Definitely conflicts with #30481 (sorry @nagisa) but wanted to get some eyes on this.