Remove most preemptive checks from GetFullPath #16478

JeremyKuhne · 2018-02-21T21:46:47Z

Builds on #16311. This makes behavior much more consistent cross-plat and allows currently blocked scenarios (notably accessing Unix volumes from Windows).

Remove all path validity checks outside of
- Null
- Embedded null
- Empty or all spaces (effectively empty)
Remove PathStartSkip helper
Use span overloads for StringBuffer usage
Clean up some comments

I've marked this as no merge until I get the test changes finished/checked-in for CoreFX. I'll link the PR when it is up.

Note that I'm going to investigate:

Removing StringBuffer (and using ValueStringBuilder)
Exposing Path directly from System.Runtime.Extensions again (as we now have a decent mirroring solution set up)

Fixes: dotnet#26359

- Remove all path validity checks outside of - Null - Embedded null - Empty or all spaces (effectively empty) - Remove PathStartSkip helper - Use span overloads for StringBuffer usage - Clean up some comments

danmoseley · 2018-02-21T22:20:09Z

src/mscorlib/shared/System/IO/PathHelper.Windows.cs

+            int rootLength = PathInternal.GetRootLength(outputBuffer);
+            bool isDevice = PathInternal.IsDevice(outputBuffer);

            StringBuffer inputBuffer = new StringBuffer(0);


BTW does it make sense for ValueStringBuilder to grow to absorb the function of STringBuffer ?

I called out that I intend to look into that in the PR comments above. It is definitely worth investigating, I just didn't want to make this change too complicated.

danmoseley · 2018-02-21T22:23:34Z

src/mscorlib/shared/System/IO/PathHelper.Windows.cs

                    if (fullPath.Length == path.Length && fullPath.StartsWith(path))
                    {
-                        // If we have the exact same string we were passed in, don't bother to allocate another string from the StringBuilder.
+                        // If we have the exact same string we were passed in, don't bother to allocate another string from the StringBuffer.


Nit, no need to fix, but if code always returns out of a conditional block

if (...) { // .. return ...; } else { . ... }

I think it's easier to read if you avoid the indent/braces ie

if (...) { // .. return ...; } . ...

I agree. I usually early out.

jkotas · 2018-02-21T22:53:49Z

src/mscorlib/shared/System/Runtime/InteropServices/StringBuffer.cs

            _length = 0;
        }
+
+        public static implicit operator ReadOnlySpan<char>(StringBuffer stringBuffer)


I think it would be better to make the cast explicit via AsReadOnlySpan method. StringBuffer is not immutable, and so you have to be careful about for how long you are going to use the Span.

jkotas · 2018-02-22T00:15:14Z

src/mscorlib/shared/System/Runtime/InteropServices/StringBuffer.cs

            _length = 0;
        }
+
+        public static explicit operator ReadOnlySpan<char>(StringBuffer stringBuffer)


Nit: We have been using AsReadOnlyMethod() in other similar places:

https://github.com/dotnet/corefx/blob/master/src/Common/src/CoreLib/System/Collections/Generic/ValueListBuilder.cs#L46
https://github.com/dotnet/corefx/pull/27250/files#diff-1a641bcf15d0f95ae2b8a25f157ecd51R71

Updates for dotnet/coreclr#16478

JeremyKuhne · 2018-02-22T04:18:40Z

Now have the test PR up. dotnet/corefx#27348.

Updates for dotnet/coreclr#16478

* Update tests for Path changes Updates for dotnet/coreclr#16478 * Add back Unix active issue (wishful thinking that this would get the same error on Unix) * Address feedback

JeremyKuhne · 2018-02-22T20:14:51Z

Tests have been updated in CoreFX. (#27348)

* Removing the colon block that tosses out paths that are not device path. Fixes: #26359 * Striping 2nd and 3rd args of PathHelper.Normalize * Build on Maryam's changes - Remove all path validity checks outside of - Null - Embedded null - Empty or all spaces (effectively empty) - Remove PathStartSkip helper - Use span overloads for StringBuffer usage - Clean up some comments * Address feedback * Tweak to match other AsSpan methods Signed-off-by: dotnet-bot <[email protected]>

* Removing the colon block that tosses out paths that are not device path. Fixes: #26359 * Striping 2nd and 3rd args of PathHelper.Normalize * Build on Maryam's changes - Remove all path validity checks outside of - Null - Embedded null - Empty or all spaces (effectively empty) - Remove PathStartSkip helper - Use span overloads for StringBuffer usage - Clean up some comments * Address feedback * Tweak to match other AsSpan methods Signed-off-by: dotnet-bot-corefx-mirror <[email protected]>

* Removing the colon block that tosses out paths that are not device path. Fixes: #26359 * Striping 2nd and 3rd args of PathHelper.Normalize * Build on Maryam's changes - Remove all path validity checks outside of - Null - Embedded null - Empty or all spaces (effectively empty) - Remove PathStartSkip helper - Use span overloads for StringBuffer usage - Clean up some comments * Address feedback * Tweak to match other AsSpan methods Signed-off-by: dotnet-bot <[email protected]>

* Update tests for Path changes Updates for dotnet/coreclr#16478 * Add back Unix active issue (wishful thinking that this would get the same error on Unix) * Address feedback

* Update tests for Path changes Updates for dotnet/coreclr#16478 * Add back Unix active issue (wishful thinking that this would get the same error on Unix) * Address feedback Commit migrated from dotnet/corefx@92afa6e

* Update tests for Path changes Updates for dotnet/coreclr#16478 * Add back Unix active issue (wishful thinking that this would get the same error on Unix) * Address feedback Commit migrated from https://github.com/dotnet/corefx/commit/dotnet/runtime@92afa6ec21817739ebf45cd66b4569a03b54d4c0 Commit migrated from dotnet/runtime@973cec3

maryamariyan and others added 3 commits February 21, 2018 11:36

Removing the colon block that tosses out paths that are not device path.

22da7f7

Fixes: dotnet#26359

Striping 2nd and 3rd args of PathHelper.Normalize

8c34745

Build on Maryam's changes

b167e58

- Remove all path validity checks outside of - Null - Embedded null - Empty or all spaces (effectively empty) - Remove PathStartSkip helper - Use span overloads for StringBuffer usage - Clean up some comments

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

JeremyKuhne added this to the 2.1.0 milestone Feb 21, 2018

JeremyKuhne self-assigned this Feb 21, 2018

JeremyKuhne requested review from Anipik, jkotas, maryamariyan and pjanotti February 21, 2018 21:46

danmoseley reviewed Feb 21, 2018

View reviewed changes

Anipik approved these changes Feb 21, 2018

View reviewed changes

jkotas reviewed Feb 21, 2018

View reviewed changes

Address feedback

1cf0673

jkotas reviewed Feb 22, 2018

View reviewed changes

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

Update tests for Path changes

1b274ef

Updates for dotnet/coreclr#16478

Tweak to match other AsSpan methods

034a976

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

Update tests for Path changes

e766d55

Updates for dotnet/coreclr#16478

JeremyKuhne mentioned this pull request Feb 22, 2018

Update tests for Path changes dotnet/corefx#27348

Merged

JeremyKuhne mentioned this pull request Feb 22, 2018

Removing the colon block that tosses out paths that are not device path. #16311

Closed

maryamariyan approved these changes Feb 22, 2018

View reviewed changes

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

Update tests for Path changes

b1415fb

Updates for dotnet/coreclr#16478

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

JeremyKuhne merged commit a0cfda0 into dotnet:master Feb 22, 2018

JeremyKuhne deleted the dialback2 branch February 22, 2018 20:14

ahsonkhan mentioned this pull request Feb 24, 2018

Fix ascii range check #16535

Merged

nkolev92 mentioned this pull request Dec 19, 2018

Address .NET Core 2.1+ System.IO Changes NuGet/Home#7588

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Remove most preemptive checks from GetFullPath #16478

Remove most preemptive checks from GetFullPath #16478

Uh oh!

JeremyKuhne commented Feb 21, 2018

Uh oh!

danmoseley Feb 21, 2018

Uh oh!

JeremyKuhne Feb 21, 2018

Uh oh!

danmoseley Feb 21, 2018

Uh oh!

JeremyKuhne Feb 21, 2018

Uh oh!

jkotas Feb 21, 2018

Uh oh!

jkotas Feb 22, 2018

Uh oh!

JeremyKuhne commented Feb 22, 2018

Uh oh!

JeremyKuhne commented Feb 22, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

Remove most preemptive checks from GetFullPath #16478

Remove most preemptive checks from GetFullPath #16478

Uh oh!

Conversation

JeremyKuhne commented Feb 21, 2018

Uh oh!

danmoseley Feb 21, 2018

Choose a reason for hiding this comment

Uh oh!

JeremyKuhne Feb 21, 2018

Choose a reason for hiding this comment

Uh oh!

danmoseley Feb 21, 2018

Choose a reason for hiding this comment

Uh oh!

JeremyKuhne Feb 21, 2018

Choose a reason for hiding this comment

Uh oh!

jkotas Feb 21, 2018

Choose a reason for hiding this comment

Uh oh!

jkotas Feb 22, 2018

Choose a reason for hiding this comment

Uh oh!

JeremyKuhne commented Feb 22, 2018

Uh oh!

JeremyKuhne commented Feb 22, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants