[SR-5582] Changed NSString.appendingPathComponent implementation on Lin… #1294
Conversation
Foundation/NSPathUtilities.swift
Outdated
| return "/" + str | ||
| } | ||
| if self == "/" { | ||
| if hasSuffix("/") { |
There was a problem hiding this comment.
I think the isEmpty check should become before the self check, as it was before. Is there a reason why they were reversed?
The change to use hasSuffix is a good one, though, and will work in the self == "/" case.
There was a problem hiding this comment.
I think the isEmpty check should become before the self check, as it was before. Is there a reason why they were reversed?
I thought about it and I think appending a path component to an empty string is less common than appending to actual path, therefore, cases where isEmpty evaluates true should be fewer. Most cases will be handled in hasSuffix and last line of the function ( self + "/" + str )
What do you think about it?
There was a problem hiding this comment.
That may be the case, but we now have different short-circuit behaviour on both platforms, and it's really unrelated to this change (which is to fix the bug where appending something onto a string that ends with / results in two // I suspect). I would prefer it to keep it as is (and minimise diffs) in order to fix this bug, and then if it is decided that the re-ordering of the test cases makes sense then it can be done. Several bugs have made it through to production where code was rewritten as:
a->something();
if (a==NULL) { abort(); }
because a compiler could 'prove' that dereferencing something implied that the variable could never be null, since otherwise would be undefined behaviour and thus the if check could be compiled out completely from the code.
While Swift doesn't suffer from this, the isEmtpy check could easily be handling such special-cases (now or in the future) and the re-ordering based on 'a thought' probably isn't the right way to tackle this issue.
Can you commit without the re-ordering on the checks so that you're just fixing the bug, and then squash all the commits so that we don't have interim mess getting in the way? We can then propose via separate PR/bug report that reordering the cases is desirable, with data to back up that particular part of the claim.
There was a problem hiding this comment.
@alblue ok, what about calling Swift.String's internal function from NSString.append... ?
TestFoundation/TestNSString.swift
Outdated
| @@ -77,7 +77,6 @@ class TestNSString : XCTestCase { | |||
| ("test_initializeWithFormat", test_initializeWithFormat), | |||
| ("test_initializeWithFormat2", test_initializeWithFormat2), | |||
| ("test_initializeWithFormat3", test_initializeWithFormat3), | |||
| ("test_deletingLastPathComponent", test_deletingLastPathComponent), | |||
There was a problem hiding this comment.
Since this is just moving the contents rather than changing, please leave it here. If necessary, you can add the test_appendingPathComponent above the test_deletingLastPathComponent to minimise the diffs.
TestFoundation/TestNSString.swift
Outdated
| @@ -753,50 +754,6 @@ class TestNSString : XCTestCase { | |||
| } | |||
| } | |||
|
|
|||
| func test_deletingLastPathComponent() { | |||
There was a problem hiding this comment.
As above, don't delete this method and move it elsewhere in the file. If you want to keep the new test with this one, insert it above here.
|
Added information about fixes to commit message; |
…ux to match Darwin's. Fixed following issues where path.appendingPathComponent(component) would produce incorrect result: path was empty and component did not start with / or path ended with / and component started with /
|
Moved |
|
@swift-ci please test |
|
@swift-ci please test and merge |
Resolves SR-5582
Changed NSString.appendingPathComponent implementation on Linux to match Darwin's
Fixed following issues where path.appendingPathComponent(component) would produce incorrect result:
path was empty and component did not start with /
or
path ended with / and component started with /
Before:
After:
Added
test_appendingPathComponent.Moved
test_deletingLastPathComponentclose to related tests (test_stringByAppendingPathExtension,test_deletingPathExtension)Before this commit, NSString and String had their own implementation and code was duplicated.
In this change,
NSString._stringByAppendingPathComponentdirectly callsString's internal function (_swiftObject._stringByAppendingPathComponent). Is this okay? @phausler