Add Path.Join() methods. #16561

JeremyKuhne · 2018-02-25T19:18:31Z

See https://github.com/dotnet/corefx/issues/25536.

cc: @bartonjs, @terrajobst, @nguerrera, @danmosemsft, @pjanotti, @Anipik

jkotas · 2018-02-25T20:47:37Z

src/mscorlib/shared/System/IO/PathInternal.cs

@@ -10,8 +10,7 @@ internal static partial class PathInternal
        /// <summary>
        /// Returns true if the path ends in a directory separator.
        /// </summary>
-        internal static bool EndsInDirectorySeparator(string path) =>
-            !string.IsNullOrEmpty(path) && IsDirectorySeparator(path[path.Length - 1]);
+        internal static bool EndsInDirectorySeparator(ReadOnlySpan<char> path) => IsDirectorySeparator(path[path.Length - 1]);


Does this need to handle path.Length == 0 ?

Not currently, but I think it is better to have it.

danmoseley · 2018-02-25T20:58:06Z

src/mscorlib/shared/System/IO/Path.cs

+                return true;
+            }
+
+            if (path2.Length == 0)


Could you avoid this duplication by acting on temp, which is whichever of path1 and path2 is not empty?

danmoseley · 2018-02-25T20:59:38Z

src/mscorlib/shared/System/IO/Path.cs

+                return true;
+            }
+
+            bool needsSeparator = !(PathInternal.EndsInDirectorySeparator(path1) || PathInternal.EndsInDirectorySeparator(path2));


Should this be checking path2 begins with directory separator?

Yep, thanks

Missing test I guess

I've got the tests, just trying to juggle multiple changes unsuccessfully.

ahsonkhan · 2018-02-27T04:29:13Z

src/mscorlib/shared/System/IO/Path.cs

+        public static string Join(ReadOnlySpan<char> path1, ReadOnlySpan<char> path2)
+        {
+            if (path1.Length == 0)
+                return path2.Length == 0 ? string.Empty : new string(path2);


Why do we have the length == 0 check here and not for path2.Length in the second if?

The string ctor already does this check, so we shouldn't need it in either case:
https://github.com/dotnet/coreclr/blob/master/src/mscorlib/src/System/String.cs#L665

Thanks, didn't know the optimization was there.

ahsonkhan · 2018-02-27T04:56:52Z

src/mscorlib/shared/System/IO/Path.cs

+            return JoinInternal(path1, path2, path3);
+        }
+
+        public static bool TryJoin(Span<char> destination, out int charsWritten, ReadOnlySpan<char> path1, ReadOnlySpan<char> path2)


How about something like this to avoid the extra path1/path2.Length == 0 checks?

In the worst case, there are 3 extra checks in the current impl.

public static bool TryJoin(Span<char> destination, out int charsWritten, ReadOnlySpan<char> path1, ReadOnlySpan<char> path2) { charsWritten = 0; if (path1.IsEmpty) { if (path2.IsEmpty) return true; if (destination.Length < path2.Length) return false; path2.CopyTo(destination); charsWritten = path2.Length; return true; } else if (path2.IsEmpty) { if (destination.Length < path1.Length) return false; path1.CopyTo(destination); charsWritten = path1.Length; return true; } Debug.Assert(!path1.IsEmpty && !path2.IsEmpty); int charsNeeded = path1.Length + path2.Length; if (!(IsDirectorySeparator(path1[path1.Length - 1]) || IsDirectorySeparator(path2[0]))) { charsNeeded++; if (destination.Length < charsNeeded) return false; path1.CopyTo(destination); destination[path1.Length] = DirectorySeparatorChar; path2.CopyTo(destination.Slice(path1.Length + 1)); } else { if (destination.Length < charsNeeded) return false; path1.CopyTo(destination); path2.CopyTo(destination.Slice(path1.Length)); } charsWritten = charsNeeded; return true; }

Because it is a little more bulky & complicated. I can't imagine optimizing checking length being worth it.

Good point. What about the needsSeparator branching? Can we just have a single if/else (which results in some code duplication).

It does seem to make a big enough difference:

How large do we expect the path length to be on average? The smaller the path length, the bigger the difference.

There will be all sorts. The first path will typically be longer than the second. I don't want to add the complexity here unless I can see a real impact in a higher level scenario. Directory.GetFiles() over a large set, for example. I'll be switching to use these methods (as opposed to our current helpers) as soon as we drop an updated CLR and we can revisit.

ahsonkhan · 2018-02-27T05:35:34Z

src/mscorlib/shared/System/IO/PathInternal.cs

@@ -10,8 +10,12 @@ internal static partial class PathInternal
        /// <summary>
        /// Returns true if the path ends in a directory separator.
        /// </summary>
-        internal static bool EndsInDirectorySeparator(string path) =>
-            !string.IsNullOrEmpty(path) && IsDirectorySeparator(path[path.Length - 1]);
+        internal static bool EndsInDirectorySeparator(ReadOnlySpan<char> path) => path.Length > 0 && IsDirectorySeparator(path[path.Length - 1]);


We seem to only call these methods when path.Length is greater than 0 anyway. Can the length check be a Debug.Assert instead?

Same with StartsWithDirectorySeparator.

That is true now, but I expect that to change, particularly as I expect to include this file in a few CoreFX libraries.

We seem to have EndsInDirectorySeparator in already:

coreclr/src/mscorlib/shared/System/IO/Path.Windows.cs

Line 221 in 22f1bc0

internal static bool EndsInDirectorySeparator(ReadOnlySpan<char> path)

But not in Path.Unix.cs (and we don't have StartsWithDirectorySeparator)

Yes, missed that. I'll get this one. I have a fair amount of cleanup to do- I intend to do so when I include the Path sources directly in System.Runtime.Extensions again.

ahsonkhan · 2018-02-27T21:23:09Z

src/mscorlib/shared/System/IO/Path.cs

@@ -444,6 +444,40 @@ public static bool TryJoin(Span<char> destination, out int charsWritten, ReadOnl
            return true;
        }

+        public static bool TryJoin(ReadOnlySpan<char> path1, ReadOnlySpan<char> path2, ReadOnlySpan<char> path3, Span<char> destination, out int charsWritten)


Any way this method can reuse the 2 path overload?

Join the first 2 paths, then combine the result with the 3rd?

I do use the other overload. Part of the contract here is that I don't write into the buffer if I can't fit the results so I can't simplify beyond what I've done.

ahsonkhan · 2018-02-27T21:25:51Z

src/mscorlib/shared/System/IO/Path.cs

@@ -444,6 +444,40 @@ public static bool TryJoin(Span<char> destination, out int charsWritten, ReadOnl
            return true;
        }

+        public static bool TryJoin(ReadOnlySpan<char> path1, ReadOnlySpan<char> path2, ReadOnlySpan<char> path3, Span<char> destination, out int charsWritten)
+        {
+            charsWritten = 0;


nit: Thoughts on moving this assignment only at the return sites? In this case, there are only two such places (line 451/467) since it is already set in other places.

Anipik · 2018-02-27T22:23:53Z

@JeremyKuhne after this #16598 gets merged we will have to change this CombineNoChecksInternal to joinInternal 8199a7e#diff-6836e42e6acde3dd212e4e6e012906ccR102

ahsonkhan · 2018-02-27T22:31:00Z

src/mscorlib/shared/System/IO/Path.cs

+            return true;
+        }
+
+        public static bool TryJoin(ReadOnlySpan<char> path1, ReadOnlySpan<char> path2, ReadOnlySpan<char> path3, Span<char> destination, out int charsWritten)


I think this can be optimized similar to the 2 path overload (probably even bigger impact), but I don't think it needs to block this PR. Feel free to file an issue/optimize outside this change.

I would suggest, however, that we do the following instead of calling the methods that do the length checks:
IsDirectorySeparator(path1[path1.Length - 1])
IsDirectorySeparator(path2[0])

ahsonkhan

Other than perf, looks good to me.

See dotnet#25536.

…ing correctly.

* Add Path.Join() methods. See dotnet#25536. * Address feedback and fix a couple issues now that I've got tests running correctly. * Update per final API approval * Fix Unix, remove redundant helper. * Merge and tweak join methods in GetFullPath(string, string) * Tweak again

* Expose Path.Join and test. Depends on dotnet/coreclr#16561 * Update for final API review changes * Address feedback * Address more feedback.

* Add Path.Join() methods. See #25536. * Address feedback and fix a couple issues now that I've got tests running correctly. * Update per final API approval * Fix Unix, remove redundant helper. * Merge and tweak join methods in GetFullPath(string, string) * Tweak again Signed-off-by: dotnet-bot <dotnet-bot@microsoft.com>

* Add Path.Join() methods. See #25536. * Address feedback and fix a couple issues now that I've got tests running correctly. * Update per final API approval * Fix Unix, remove redundant helper. * Merge and tweak join methods in GetFullPath(string, string) * Tweak again Signed-off-by: dotnet-bot-corefx-mirror <dotnet-bot@microsoft.com>

* Add Path.Join() methods. See #25536. * Address feedback and fix a couple issues now that I've got tests running correctly. * Update per final API approval * Fix Unix, remove redundant helper. * Merge and tweak join methods in GetFullPath(string, string) * Tweak again Signed-off-by: dotnet-bot <dotnet-bot@microsoft.com>

* Add Path.Join() methods. See #25536. * Address feedback and fix a couple issues now that I've got tests running correctly. * Update per final API approval * Fix Unix, remove redundant helper. * Merge and tweak join methods in GetFullPath(string, string) * Tweak again Signed-off-by: dotnet-bot-corefx-mirror <dotnet-bot@microsoft.com>

* Add Path.Join() methods. See #25536. * Address feedback and fix a couple issues now that I've got tests running correctly. * Update per final API approval * Fix Unix, remove redundant helper. * Merge and tweak join methods in GetFullPath(string, string) * Tweak again Signed-off-by: dotnet-bot <dotnet-bot@microsoft.com>

* Expose Path.Join and test. Depends on dotnet/coreclr#16561 * Update for final API review changes * Address feedback * Address more feedback. Commit migrated from dotnet/corefx@0a092a2

JeremyKuhne added * NO MERGE * The PR is not ready for merge yet (see discussion for detailed reasons) area-System.IO labels Feb 25, 2018

JeremyKuhne added this to the 2.1.0 milestone Feb 25, 2018

JeremyKuhne self-assigned this Feb 25, 2018

jkotas reviewed Feb 25, 2018

View reviewed changes

danmoseley approved these changes Feb 25, 2018

View reviewed changes

JeremyKuhne added a commit to JeremyKuhne/corefx that referenced this pull request Feb 26, 2018

Expose Path.Join and test. Depends on dotnet/coreclr#16561

ea0bfb9

ahsonkhan reviewed Feb 27, 2018

View reviewed changes

JeremyKuhne force-pushed the pathjoin branch from 3ec97fb to 4a38da8 Compare February 27, 2018 21:18

JeremyKuhne removed the * NO MERGE * The PR is not ready for merge yet (see discussion for detailed reasons) label Feb 27, 2018

JeremyKuhne added a commit to JeremyKuhne/corefx that referenced this pull request Feb 27, 2018

Expose Path.Join and test. Depends on dotnet/coreclr#16561

403ab81

ahsonkhan reviewed Feb 27, 2018

View reviewed changes

JeremyKuhne mentioned this pull request Feb 27, 2018

Expose Path.Join and tests dotnet/corefx#27522

Merged

ahsonkhan reviewed Feb 27, 2018

View reviewed changes

danmoseley requested a review from Anipik February 27, 2018 21:34

Anipik approved these changes Feb 27, 2018

View reviewed changes

ahsonkhan reviewed Feb 27, 2018

View reviewed changes

ahsonkhan approved these changes Feb 27, 2018

View reviewed changes

JeremyKuhne added 5 commits February 27, 2018 15:57

Add Path.Join() methods.

48d52d8

See dotnet#25536.

Address feedback and fix a couple issues now that I've got tests runn…

b71b0e9

…ing correctly.

Update per final API approval

6e07cb4

Fix Unix, remove redundant helper.

a6fc5c9

Merge and tweak join methods in GetFullPath(string, string)

2bba548

JeremyKuhne force-pushed the pathjoin branch from 2a1812a to 2bba548 Compare February 28, 2018 00:28

Tweak again

3cb5dec

JeremyKuhne merged commit f1fee6d into dotnet:master Feb 28, 2018

JeremyKuhne added a commit to JeremyKuhne/corefx that referenced this pull request Feb 28, 2018

Expose Path.Join and test. Depends on dotnet/coreclr#16561

6ad515b

JeremyKuhne added a commit to dotnet/corefx that referenced this pull request Feb 28, 2018

Expose Path.Join and tests (#27522)

0a092a2

* Expose Path.Join and test. Depends on dotnet/coreclr#16561 * Update for final API review changes * Address feedback * Address more feedback.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add Path.Join() methods. #16561

Add Path.Join() methods. #16561

JeremyKuhne commented Feb 25, 2018 •

edited

Loading

jkotas Feb 25, 2018

JeremyKuhne Feb 25, 2018

danmoseley Feb 25, 2018

danmoseley Feb 25, 2018

JeremyKuhne Feb 25, 2018

danmoseley Feb 25, 2018

JeremyKuhne Feb 25, 2018

ahsonkhan Feb 27, 2018

JeremyKuhne Feb 27, 2018

ahsonkhan Feb 27, 2018

JeremyKuhne Feb 27, 2018

ahsonkhan Feb 27, 2018

ahsonkhan Feb 27, 2018

JeremyKuhne Feb 27, 2018

ahsonkhan Feb 27, 2018

JeremyKuhne Feb 27, 2018

ahsonkhan Feb 27, 2018

JeremyKuhne Feb 27, 2018

ahsonkhan Feb 27, 2018

JeremyKuhne Feb 27, 2018

ahsonkhan Feb 27, 2018

Anipik commented Feb 27, 2018

ahsonkhan Feb 27, 2018 •

edited

Loading

ahsonkhan left a comment

Add Path.Join() methods. #16561

Add Path.Join() methods. #16561

Conversation

JeremyKuhne commented Feb 25, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Anipik commented Feb 27, 2018

ahsonkhan Feb 27, 2018 • edited Loading

Choose a reason for hiding this comment

ahsonkhan left a comment

Choose a reason for hiding this comment

JeremyKuhne commented Feb 25, 2018 •

edited

Loading

ahsonkhan Feb 27, 2018 •

edited

Loading