-
Notifications
You must be signed in to change notification settings - Fork 10.5k
Add withVaList() for environments without ObjC #339
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
Conversation
|
At least I have confirmed that va_list works on Linux x86-64. |
There are edge cases where OS X diverges from Linux with unions, but I doubt we handle those properly in |
The OS X x86_64 and Linux x86_64 calling conventions are identical or nearly so. I have no idea how va_list itself is stored on Linux but if it operates correctly then it should be good enough. @cielavenir, you should remove the |
It is taking too much time to build swift even with 8GB memory... Do you have a way for quick testing...? |
@@ -429,4 +430,388 @@ final public class VaListBuilder { | |||
|
|||
#endif | |||
|
|||
#else | |||
|
|||
import CoreFoundation |
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.
We can't use CoreFoundation in the standard library, that creates dependency loops.
I'm sorry, but the code looks like it was copy-pasted from the other side of the |
Although I cannot do so much for VaListBuilder, other parts could be compressed. |
@cielavenir Please use |
initializeFrom source must be UnsafeMutablePointer, which is not available without Builtin.addressof. |
Doesn't just |
You can also use |
Thank you. Now I was able to reduce duplication as much as possible. |
args.append(AutoreleasingUnsafeMutablePointer<AnyObject>( | ||
UnsafeMutablePointer<AnyObject>(bitPattern: 0x1234_5674))) | ||
//args.append(AutoreleasingUnsafeMutablePointer<AnyObject>( | ||
// UnsafeMutablePointer<AnyObject>(bitPattern: 0x1234_5674))) |
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.
Please don't remove the test. Instead, conditionalize it on #if _runtime(_ObjC)
.
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.
Do you mean this? This c_vprintf stuff is exactly duplicate but I cannot avoid now.
#if _runtime(_ObjC)
args.append(AutoreleasingUnsafeMutablePointer<AnyObject>(
UnsafeMutablePointer<AnyObject>(bitPattern: 0x1234_5674)))
// CHECK: {{pointers: '(0x)?0*12345670' '(0x)?0*12345671' '(0x)?0*12345672' '(0x)?0*12345673' '(0x)?0*12345674'}}
withVaList(args) {
c_vprintf(format, $0)
}
#else
// CHECK: {{pointers: '(0x)?0*12345670' '(0x)?0*12345671' '(0x)?0*12345672' '(0x)?0*12345673'}}
withVaList(args) {
c_vprintf(format, $0)
}
#endif
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.
Yes... except that won't work with FileCheck, it does not know about #if
. We need to do something like this:
// RUN: %target-run-stdlib-swift -parse-stdlib %s | FileCheck %s --check-prefix=CHECK --check-prefix=CHECK-%target-runtime
#if _runtime(_ObjC)
... code
// CHECK-objc: ...
#else
... code
// CHECK-native: ...
#endif
Now I have reverted most stuff and the compilation is still fine. But,
There is |
Note: I promise all commits will be rebased after approval. |
We can't depend on CoreFoundation in the standard library, that would create circular dependencies. |
OK. How does OSX version achieve vprintf? The Note:
|
Uh... This code works... Why test is not working? I'm lost...
|
44f677e
to
3d81f46
Compare
Now I was able to tweak (the header area of) VarArgs test and have confirmed there are no regressions. So, I carefully rebased commits and now the difference is so small now. |
Good |
Implementation looks good. The test can be simplified further. You can get rid of the CHECK complications around AutoreleasingUnsafeMutablePointer by using the same placeholder technique that you used for CGFloat. c_vprintf() is a historical artifact that can be removed now. CGFloat can be handled using a typealias instead. Do it like this:
Then just use vprintf() and CGFloat everywhere with no other runtime checks. I tested this on the OS X side and it works there. |
Updated according to your advice. Thank you. |
Just let us know when you're happy with the patch :) |
Hopefully my machine started up (for now) and I was able to test current patch. It is all right to be merged. |
Looks good. Test passes on OS X. Thanks! |
Add withVaList() for environments without ObjC
Thank you so much! |
protect unistd.h inclusion with HAVE_UNISTD_H
Remove apparently fixed Dollar SourceKit XFAIL
Currently Linux edition lacks getVaList() and withVaList().
I agree that getVaList() is not available due to memory management model, but I believe that withVaList() is possible.
Sample code is here: http://qiita.com/cielavenir/items/2598d47b97a7c9caf970