Skip to content
This repository has been archived by the owner on Jan 23, 2023. It is now read-only.

Add Path.Join() methods. #16561

Merged
merged 6 commits into from
Feb 28, 2018
Merged

Add Path.Join() methods. #16561

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
48d52d8
Add Path.Join() methods.
JeremyKuhne Feb 25, 2018
b71b0e9
Address feedback and fix a couple issues now that I've got tests runn…
JeremyKuhne Feb 26, 2018
6e07cb4
Update per final API approval
JeremyKuhne Feb 27, 2018
a6fc5c9
Fix Unix, remove redundant helper.
JeremyKuhne Feb 27, 2018
2bba548
Merge and tweak join methods in GetFullPath(string, string)
JeremyKuhne Feb 28, 2018
3cb5dec
Tweak again
JeremyKuhne Feb 28, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion src/mscorlib/shared/System/IO/Path.Unix.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -61,7 +61,7 @@ public static string GetFullPath(string path, string basePath)
if (IsPathFullyQualified(path))
return GetFullPath(path);

return GetFullPath(CombineNoChecks(basePath, path));
return GetFullPath(CombineInternal(basePath, path));
}

private static string RemoveLongPathPrefix(string path)
Expand Down
24 changes: 10 additions & 14 deletions src/mscorlib/shared/System/IO/Path.Windows.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -80,34 +80,38 @@ public static string GetFullPath(string path, string basePath)
// Path is current drive rooted i.e. starts with \:
// "\Foo" and "C:\Bar" => "C:\Foo"
// "\Foo" and "\\?\C:\Bar" => "\\?\C:\Foo"
combinedPath = CombineNoChecks(GetPathRoot(basePath), path.AsSpan().Slice(1));
combinedPath = Join(GetPathRoot(basePath.AsSpan()), path);
}
else if (length >= 2 && PathInternal.IsValidDriveChar(path[0]) && path[1] == PathInternal.VolumeSeparatorChar)
{
// Drive relative paths
Debug.Assert(length == 2 || !PathInternal.IsDirectorySeparator(path[2]));

if (StringSpanHelpers.Equals(GetVolumeName(path.AsSpan()), GetVolumeName(basePath.AsSpan())))
if (StringSpanHelpers.Equals(GetVolumeName(path), GetVolumeName(basePath)))
{
// Matching root
// "C:Foo" and "C:\Bar" => "C:\Bar\Foo"
// "C:Foo" and "\\?\C:\Bar" => "\\?\C:\Bar\Foo"
combinedPath = CombineNoChecks(basePath, path.AsSpan().Slice(2));
combinedPath = Join(basePath, path.AsSpan().Slice(2));
}
else
{
// No matching root, root to specified drive
// "D:Foo" and "C:\Bar" => "D:Foo"
// "D:Foo" and "\\?\C:\Bar" => "\\?\D:\Foo"
combinedPath = PathInternal.IsDevice(basePath) ? CombineNoChecksInternal(basePath.AsSpan().Slice(0, 4), path.AsSpan().Slice(0, 2), @"\", path.AsSpan().Slice(2)) : path.Insert(2, "\\");
combinedPath = !PathInternal.IsDevice(basePath)
? path.Insert(2, @"\")
: length == 2
? JoinInternal(basePath.AsSpan().Slice(0, 4), path, @"\")
: JoinInternal(basePath.AsSpan().Slice(0, 4), path.AsSpan().Slice(0, 2), @"\", path.AsSpan().Slice(2));
}
}
else
{
// "Simple" relative path
// "Foo" and "C:\Bar" => "C:\Bar\Foo"
// "Foo" and "\\?\C:\Bar" => "\\?\C:\Bar\Foo"
combinedPath = CombineNoChecks(basePath, path);
combinedPath = JoinInternal(basePath, path);
}

// Device paths are normalized by definition, so passing something of this format
Expand Down Expand Up @@ -215,20 +219,12 @@ internal static ReadOnlySpan<char> GetVolumeName(ReadOnlySpan<char> path)
return TrimEndingDirectorySeparator(root); // e.g. "C:"
}

/// <summary>
/// Returns true if the path ends in a directory separator.
/// </summary>
internal static bool EndsInDirectorySeparator(ReadOnlySpan<char> path)
{
return path.Length > 0 && PathInternal.IsDirectorySeparator(path[path.Length - 1]);
}

/// <summary>
/// Trims the ending directory separator if present.
/// </summary>
/// <param name="path"></param>
internal static ReadOnlySpan<char> TrimEndingDirectorySeparator(ReadOnlySpan<char> path) =>
EndsInDirectorySeparator(path) ?
PathInternal.EndsInDirectorySeparator(path) ?
path.Slice(0, path.Length - 1) :
path;

Expand Down
186 changes: 119 additions & 67 deletions src/mscorlib/shared/System/IO/Path.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -262,7 +262,6 @@ public static bool IsPathFullyQualified(ReadOnlySpan<char> path)
return !PathInternal.IsPartiallyQualified(path);
}


/// <summary>
/// Tests if a path's file name includes a file extension. A trailing period
/// is not considered an extension.
Expand Down Expand Up @@ -296,23 +295,23 @@ public static string Combine(string path1, string path2)
if (path1 == null || path2 == null)
throw new ArgumentNullException((path1 == null) ? nameof(path1) : nameof(path2));

return CombineNoChecks(path1, path2);
return CombineInternal(path1, path2);
}

public static string Combine(string path1, string path2, string path3)
{
if (path1 == null || path2 == null || path3 == null)
throw new ArgumentNullException((path1 == null) ? nameof(path1) : (path2 == null) ? nameof(path2) : nameof(path3));

return CombineNoChecks(path1, path2, path3);
return CombineInternal(path1, path2, path3);
}

public static string Combine(string path1, string path2, string path3, string path4)
{
if (path1 == null || path2 == null || path3 == null || path4 == null)
throw new ArgumentNullException((path1 == null) ? nameof(path1) : (path2 == null) ? nameof(path2) : (path3 == null) ? nameof(path3) : nameof(path4));

return CombineNoChecks(path1, path2, path3, path4);
return CombineInternal(path1, path2, path3, path4);
}

public static string Combine(params string[] paths)
Expand Down Expand Up @@ -383,11 +382,102 @@ public static string Combine(params string[] paths)
return StringBuilderCache.GetStringAndRelease(finalPath);
}

/// <summary>
/// Combines two paths. Does no validation of paths, only concatenates the paths
/// and places a directory separator between them if needed.
/// </summary>
private static string CombineNoChecks(ReadOnlySpan<char> first, ReadOnlySpan<char> second)
// Unlike Combine(), Join() methods do not consider rooting. They simply combine paths, ensuring that there
// is a directory separator between them.

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

return JoinInternal(path1, path2);
}

public static string Join(ReadOnlySpan<char> path1, ReadOnlySpan<char> path2, ReadOnlySpan<char> path3)
{
if (path1.Length == 0)
return Join(path2, path3);

if (path2.Length == 0)
return Join(path1, path3);

if (path3.Length == 0)
return Join(path1, path2);

return JoinInternal(path1, path2, path3);
}

public static bool TryJoin(ReadOnlySpan<char> path1, ReadOnlySpan<char> path2, Span<char> destination, out int charsWritten)
{
charsWritten = 0;
if (path1.Length == 0 && path2.Length == 0)
return true;

if (path1.Length == 0 || path2.Length == 0)
{
ref ReadOnlySpan<char> pathToUse = ref path1.Length == 0 ? ref path2 : ref path1;
if (destination.Length < pathToUse.Length)
{
return false;
}

pathToUse.CopyTo(destination);
charsWritten = pathToUse.Length;
return true;
}

bool needsSeparator = !(PathInternal.EndsInDirectorySeparator(path1) || PathInternal.StartsWithDirectorySeparator(path2));
int charsNeeded = path1.Length + path2.Length + (needsSeparator ? 1 : 0);
if (destination.Length < charsNeeded)
return false;

path1.CopyTo(destination);
if (needsSeparator)
destination[path1.Length] = DirectorySeparatorChar;

path2.CopyTo(destination.Slice(path1.Length + (needsSeparator ? 1 : 0)));

charsWritten = charsNeeded;
return true;
}

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

@ahsonkhan ahsonkhan Feb 27, 2018 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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])

{
charsWritten = 0;
if (path1.Length == 0 && path2.Length == 0 && path3.Length == 0)
return true;

if (path1.Length == 0)
return TryJoin(path2, path3, destination, out charsWritten);
if (path2.Length == 0)
return TryJoin(path1, path3, destination, out charsWritten);
if (path3.Length == 0)
return TryJoin(path1, path2, destination, out charsWritten);

int neededSeparators = PathInternal.EndsInDirectorySeparator(path1) || PathInternal.StartsWithDirectorySeparator(path2) ? 0 : 1;
bool needsSecondSeparator = !(PathInternal.EndsInDirectorySeparator(path2) || PathInternal.StartsWithDirectorySeparator(path3));
if (needsSecondSeparator)
neededSeparators++;

int charsNeeded = path1.Length + path2.Length + path3.Length + neededSeparators;
if (destination.Length < charsNeeded)
return false;

bool result = TryJoin(path1, path2, destination, out charsWritten);
Debug.Assert(result, "should never fail joining first two paths");

if (needsSecondSeparator)
destination[charsWritten++] = DirectorySeparatorChar;

path3.CopyTo(destination.Slice(charsWritten));
charsWritten += path3.Length;

return true;
}

private static string CombineInternal(ReadOnlySpan<char> first, ReadOnlySpan<char> second)
{
if (first.Length == 0)
return second.Length == 0
Expand All @@ -400,10 +490,10 @@ private static string CombineNoChecks(ReadOnlySpan<char> first, ReadOnlySpan<cha
if (IsPathRooted(second))
return new string(second);

return CombineNoChecksInternal(first, second);
return JoinInternal(first, second);
}

private static string CombineNoChecks(string first, string second)
private static string CombineInternal(string first, string second)
{
if (string.IsNullOrEmpty(first))
return second;
Expand All @@ -414,86 +504,48 @@ private static string CombineNoChecks(string first, string second)
if (IsPathRooted(second.AsSpan()))
return second;

return CombineNoChecksInternal(first, second);
}

private static string CombineNoChecks(ReadOnlySpan<char> first, ReadOnlySpan<char> second, ReadOnlySpan<char> third)
{
if (first.Length == 0)
return CombineNoChecks(second, third);
if (second.Length == 0)
return CombineNoChecks(first, third);
if (third.Length == 0)
return CombineNoChecks(first, second);

if (IsPathRooted(third))
return new string(third);
if (IsPathRooted(second))
return CombineNoChecks(second, third);

return CombineNoChecksInternal(first, second, third);
return JoinInternal(first, second);
}

private static string CombineNoChecks(string first, string second, string third)
private static string CombineInternal(string first, string second, string third)
{
if (string.IsNullOrEmpty(first))
return CombineNoChecks(second, third);
return CombineInternal(second, third);
if (string.IsNullOrEmpty(second))
return CombineNoChecks(first, third);
return CombineInternal(first, third);
if (string.IsNullOrEmpty(third))
return CombineNoChecks(first, second);
return CombineInternal(first, second);

if (IsPathRooted(third.AsSpan()))
return third;
if (IsPathRooted(second.AsSpan()))
return CombineNoChecks(second, third);

return CombineNoChecksInternal(first, second, third);
}

private static string CombineNoChecks(ReadOnlySpan<char> first, ReadOnlySpan<char> second, ReadOnlySpan<char> third, ReadOnlySpan<char> fourth)
{
if (first.Length == 0)
return CombineNoChecks(second, third, fourth);
if (second.Length == 0)
return CombineNoChecks(first, third, fourth);
if (third.Length == 0)
return CombineNoChecks(first, second, fourth);
if (fourth.Length == 0)
return CombineNoChecks(first, second, third);

if (IsPathRooted(fourth))
return new string(fourth);
if (IsPathRooted(third))
return CombineNoChecks(third, fourth);
if (IsPathRooted(second))
return CombineNoChecks(second, third, fourth);
return CombineInternal(second, third);

return CombineNoChecksInternal(first, second, third, fourth);
return JoinInternal(first, second, third);
}

private static string CombineNoChecks(string first, string second, string third, string fourth)
private static string CombineInternal(string first, string second, string third, string fourth)
{
if (string.IsNullOrEmpty(first))
return CombineNoChecks(second, third, fourth);
return CombineInternal(second, third, fourth);
if (string.IsNullOrEmpty(second))
return CombineNoChecks(first, third, fourth);
return CombineInternal(first, third, fourth);
if (string.IsNullOrEmpty(third))
return CombineNoChecks(first, second, fourth);
return CombineInternal(first, second, fourth);
if (string.IsNullOrEmpty(fourth))
return CombineNoChecks(first, second, third);
return CombineInternal(first, second, third);

if (IsPathRooted(fourth.AsSpan()))
return fourth;
if (IsPathRooted(third.AsSpan()))
return CombineNoChecks(third, fourth);
return CombineInternal(third, fourth);
if (IsPathRooted(second.AsSpan()))
return CombineNoChecks(second, third, fourth);
return CombineInternal(second, third, fourth);

return CombineNoChecksInternal(first, second, third, fourth);
return JoinInternal(first, second, third, fourth);
}

private unsafe static string CombineNoChecksInternal(ReadOnlySpan<char> first, ReadOnlySpan<char> second)
private unsafe static string JoinInternal(ReadOnlySpan<char> first, ReadOnlySpan<char> second)
{
Debug.Assert(first.Length > 0 && second.Length > 0, "should have dealt with empty paths");

Expand All @@ -515,7 +567,7 @@ private unsafe static string CombineNoChecksInternal(ReadOnlySpan<char> first, R
}
}

private unsafe static string CombineNoChecksInternal(ReadOnlySpan<char> first, ReadOnlySpan<char> second, ReadOnlySpan<char> third)
private unsafe static string JoinInternal(ReadOnlySpan<char> first, ReadOnlySpan<char> second, ReadOnlySpan<char> third)
{
Debug.Assert(first.Length > 0 && second.Length > 0 && third.Length > 0, "should have dealt with empty paths");

Expand Down Expand Up @@ -543,7 +595,7 @@ private unsafe static string CombineNoChecksInternal(ReadOnlySpan<char> first, R
}
}

private unsafe static string CombineNoChecksInternal(ReadOnlySpan<char> first, ReadOnlySpan<char> second, ReadOnlySpan<char> third, ReadOnlySpan<char> fourth)
private unsafe static string JoinInternal(ReadOnlySpan<char> first, ReadOnlySpan<char> second, ReadOnlySpan<char> third, ReadOnlySpan<char> fourth)
{
Debug.Assert(first.Length > 0 && second.Length > 0 && third.Length > 0 && fourth.Length > 0, "should have dealt with empty paths");

Expand Down
8 changes: 6 additions & 2 deletions src/mscorlib/shared/System/IO/PathInternal.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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]);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


/// <summary>
/// Returns true if the path starts in a directory separator.
/// </summary>
internal static bool StartsWithDirectorySeparator(ReadOnlySpan<char> path) => path.Length > 0 && IsDirectorySeparator(path[0]);

/// <summary>
/// Get the common path length from the start of the string.
Expand Down