Skip to content

Conversation

@carlossanlop
Copy link
Contributor

@carlossanlop carlossanlop commented Jan 25, 2020

Fixes #2162

@carlossanlop carlossanlop added this to the 5.0 milestone Jan 25, 2020
@carlossanlop carlossanlop self-assigned this Jan 25, 2020
@MihaZupan
Copy link
Member

The purpose of TryRemoveRedundantSegments accepting a Span destination is to avoid allocations. As such, it can't delegate to the string-returning methods and only do a copy.

@carlossanlop
Copy link
Contributor Author

I'll close this issue while it's in draft mode. I'll reopen when the unit tests are ready.

@carlossanlop carlossanlop reopened this Feb 22, 2020
@carlossanlop carlossanlop marked this pull request as ready for review February 22, 2020 04:43
@carlossanlop
Copy link
Contributor Author

@jkotas Need your help - I cannot rebase because your change #31991 deleted the file ref/System.Runtime.Extensions.cs.

Where should I put my new method references now?

@jkotas
Copy link
Member

jkotas commented Feb 22, 2020

Where should I put my new method references now?

src/libraries/System.Runtime/ref/System.Runtime.cs

Copy link
Member

Choose a reason for hiding this comment

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

ValueStringBuilder is meant to be used with stackallocated buffer for sizes up to certain limit. Getting a pooled buffer has too much overhead for small buffers.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@JeremyKuhne - I'm investigating these tests where I put a comment next to them, because their behavior was slightly different than expected.

I also noticed the resulting path is different in Windows than in Unix, because of the separator char used.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I talked with Jeremy about the expected behavior on each edge case. I am adding tests to verify them.

@carlossanlop
Copy link
Contributor Author

carlossanlop commented Apr 21, 2020

Currently blocked by a bug I found in Path.GetPathRoot, which is the reason why some of the unit tests are failing in the CI: #35260.
I'm investigating it.

@carlossanlop carlossanlop requested a review from a team April 22, 2020 03:57
@carlossanlop carlossanlop removed the request for review from a team April 22, 2020 19:56
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

@carlossanlop
Copy link
Contributor Author

I'll close this while I address the Unix failures.

@ghost ghost locked as resolved and limited conversation to collaborators Dec 11, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Add Path.RemoveRelativeSegments Api

9 participants