Skip to content
Closed
Prev Previous commit
Made some methods span based. Added a missing PlatformSpecific attrib…
…ute.
  • Loading branch information
carlossanlop committed Apr 22, 2020
commit 3cfbe4576c8606ffb556b271ed74b7773e826779
19 changes: 13 additions & 6 deletions src/libraries/System.Private.CoreLib/src/System/IO/Path.Windows.cs
Original file line number Diff line number Diff line change
Expand Up @@ -213,11 +213,18 @@ public static bool IsPathRooted(ReadOnlySpan<char> path)
if (PathInternal.IsEffectivelyEmpty(path.AsSpan()))
return null;

ReadOnlySpan<char> result = GetPathRoot(path.AsSpan());
if (path!.Length == result.Length)
return PathInternal.NormalizeDirectorySeparators(path);
int rootLength = PathInternal.GetRootLength(path.AsSpan());
if (rootLength <= 0)
return null;

return PathInternal.NormalizeDirectorySeparators(result.ToString());
Span<char> destination = stackalloc char[rootLength];
// Fails when path already normalized (or empty, but won't happen here)
if (PathInternal.TryNormalizeDirectorySeparators(path.AsSpan(0, rootLength), destination, out int charsWritten))
{
return destination.Slice(0, charsWritten).ToString();
}
// Reaching here means the path was already normalized
return path!.Substring(0, rootLength);
}

/// <remarks>
Expand All @@ -228,8 +235,8 @@ public static ReadOnlySpan<char> GetPathRoot(ReadOnlySpan<char> path)
if (PathInternal.IsEffectivelyEmpty(path))
return ReadOnlySpan<char>.Empty;

int pathRoot = PathInternal.GetRootLength(path);
return pathRoot <= 0 ? ReadOnlySpan<char>.Empty : path.Slice(0, pathRoot);
int rootLength = PathInternal.GetRootLength(path);
return rootLength <= 0 ? ReadOnlySpan<char>.Empty : path.Slice(0, rootLength);
}

/// <summary>Gets whether the system is case-sensitive.</summary>
Expand Down
13 changes: 12 additions & 1 deletion src/libraries/System.Private.CoreLib/src/System/IO/Path.cs
Original file line number Diff line number Diff line change
Expand Up @@ -107,7 +107,18 @@ public static partial class Path
return null;

int end = GetDirectoryNameOffset(path.AsSpan());
return end >= 0 ? PathInternal.NormalizeDirectorySeparators(path.Substring(0, end)) : null;

if (end >= 0)
{
Span<char> destination = stackalloc char[end];
// Fails when empty or already normalized
if (PathInternal.TryNormalizeDirectorySeparators(path.AsSpan(0, end), destination, out int charsWritten))
{
return destination.Slice(0, charsWritten).ToString();
}
return path.Substring(0, end);
}
return null;
}

/// <summary>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -314,7 +314,7 @@ internal static bool IsDirectorySeparator(char c)
/// Like the current NormalizePath this will not try and analyze periods/spaces within directory segments.
/// </summary>
/// <remarks>
/// The only callers that used to use Path.Normalize(fullCheck=false) were Path.GetDirectoryName() and Path.GetPathRoot(). Both usages do
/// The only callers that used to use Path.Normalize(fullCheck=false) were Path.GetDirectoryName() and Path.GetPathRoot(string). Both usages do
/// not need trimming of trailing whitespace here.
///
/// GetPathRoot() could technically skip normalizing separators after the second segment- consider as a future optimization.
Expand All @@ -341,6 +341,22 @@ internal static bool IsDirectorySeparator(char c)
if (string.IsNullOrEmpty(path))
return path;

Span<char> destination = stackalloc char[path.Length];
Copy link
Member

Choose a reason for hiding this comment

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

Unbounded stackallocs are prohibited in dotnet/runtime libraries

Copy link
Member

Choose a reason for hiding this comment

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

@stephentoub I don't see record of a discussion about trying to catch this with an analyzer. It's not generally possible to be sure (either way) whether it's unbounded. But an analyzer that forced a pattern of passing a constant value would be doable.

Looking at all our stackallocs, almost all of them already pass a constant directly or via a constant field or local. The few exceptions are not self evidently safe by eyeball, eg.,

        private static unsafe int EncryptDecryptHelper(OP op, ISSPIInterface secModule, SafeDeleteContext context, Span<SecurityBuffer> input, uint sequenceNumber)
        {
            Interop.SspiCli.SecBufferDesc sdcInOut = new Interop.SspiCli.SecBufferDesc(input.Length);
            Span<Interop.SspiCli.SecBuffer> unmanagedBuffer = stackalloc Interop.SspiCli.SecBuffer[input.Length];

and it would be easy for one of them to be wrong. Would it be reasonable to have an analyzer that required them to be rewritten in constant terms, or at least something the analyzer could recognize like

            Span<Interop.SspiCli.SecBuffer> unmanagedBuffer = stackalloc Interop.SspiCli.SecBuffer[Math.Min(input.Length, BufferSize)];

Copy link
Member

Choose a reason for hiding this comment

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

Moved to #35382

if (!TryNormalizeDirectorySeparators(path.AsSpan(), destination, out int charsWritten))
{
return path;
}

return destination.Slice(0, charsWritten).ToString();
}

internal static bool TryNormalizeDirectorySeparators(ReadOnlySpan<char> path, Span<char> destination, out int charsWritten)
{
charsWritten = 0;

if (IsEffectivelyEmpty(path))
return false;

char current;

// Make a pass to see if we need to normalize so we can potentially skip allocating
Expand All @@ -360,7 +376,7 @@ internal static bool IsDirectorySeparator(char c)
}

if (normalized)
return path;
return false;

var builder = new ValueStringBuilder(stackalloc char[MaxShortPath]);

Expand Down Expand Up @@ -391,7 +407,7 @@ internal static bool IsDirectorySeparator(char c)
builder.Append(current);
}

return builder.ToString();
return builder.TryCopyTo(destination, out charsWritten);
}

/// <summary>
Expand All @@ -411,5 +427,6 @@ internal static bool IsEffectivelyEmpty(ReadOnlySpan<char> path)
}
return true;
}

}
}
Original file line number Diff line number Diff line change
Expand Up @@ -437,6 +437,7 @@ public static void WindowsValid_String(string path, string expected)
[MemberData(nameof(WindowsValidCurrentDirectoryData))]
[MemberData(nameof(WindowsCombinedRedundantData))]
[MemberData(nameof(WindowsDuplicateSeparatorsData))]
[PlatformSpecific(TestPlatforms.Windows)]
public static void WindowsValid_Span(string path, string expected)
{
foreach (string prefix in s_Prefixes)
Expand Down