-
Notifications
You must be signed in to change notification settings - Fork 10.5k
Port swiftSyntax to Windows #7588
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
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.
To add a test:
- Add an OwnedString.cpp to ./unittests
- Don't forget the license header; see other files in the directory.
- Include OwnedString.cpp in ./unittests/CMakeLists.txt
include/swift/Basic/OwnedString.h
Outdated
|
||
return data; | ||
} | ||
|
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.
Let's do the following, please.
- Change
OwnedString(const char *Data, size_t Length, StringOwnership Ownership)
to:
OwnedString(const char *Data, size_t Length)
- assume
StringOwnership::Unowned
- assume
- Move
copyString(const char *s, size_t length)
toOwnedString OwnedString::copy()
- Add rigorous testing for copying. This is an absolute must if we are going to be implementing the equivalent to
strndup
.
@bitjammer It is also important that the test follows the naming convention. Specifically, it should be OwnedStringTest.cpp like all of the other tests in ./unittests. |
Okay, so I'm getting these errors "pointer being freed was not allocated". Odd, considering the only time we are I'm not really sure where I'm going wrong - If you guys have any ideas, I'd appreciate it! |
You took the copy out of OwnedString(const OwnedString &Other), but you're still reusing the StringOwnership argument. You need to do a copy there in that constructor if the ownership is Copied, otherwise you'll free the same pointer twice. |
Thanks David - you're right! I've updated the PR and added some unit tests that pass on Windows. I haven't got anything in |
@swift-ci please smoke test |
include/swift/Basic/OwnedString.h
Outdated
@@ -88,12 +103,13 @@ class OwnedString { | |||
} | |||
|
|||
~OwnedString() { | |||
if (Ownership == StringOwnership::Copied) | |||
if (Ownership == StringOwnership::Copied && Data) { |
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.
This shouldn't be necessary; free
accepts a null pointer.
@swift-ci please smoke test |
1 similar comment
@swift-ci please smoke test |
@swift-ci Please asan test |
It looks like this had been kicked off: https://ci.swift.org/view/All/job/swift-PR-osx-ASAN-test/47/ |
(But no progress reported) |
its due to wrong sha Setting status of d0eed9e97782d1b30b6481341a92f4a1b045510a to PENDING with url https://ci.swift.org/job/swift-PR-osx-ASAN-test/47/ and message: 'Build started.' |
@swift-ci Please asan test |
Setting status of b564a91 to PENDING with url https://ci.swift.org/job/swift-PR-osx-ASAN-test/48/ and message: 'Build started.' This worked now, sorry for the trouble. |
|
Is the ASAN failure related? |
This should be fixed now. @swift-ci Please asan test |
I will re-trigger this asan job right after I update the Xcode. |
@swift-ci Please asan test |
Great! Thanks Mishal. Tests have passed, let me know if you have any more comments |
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.
Looks good - thanks so much for taking the time to do this right. I appreciate it.
strndup
is not available on Windows. Windows does providestrdup
but that isn't the behaviour we want unfortunately.So roll out our own solution
@bitjammer