Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Need Span overloads for Path APIs #24264

Closed
JeremyKuhne opened this issue Nov 28, 2017 · 9 comments
Closed

Need Span overloads for Path APIs #24264

JeremyKuhne opened this issue Nov 28, 2017 · 9 comments
Assignees
Labels
api-approved API was approved in API review, it can be implemented area-System.IO
Milestone

Comments

@JeremyKuhne
Copy link
Member

In order to reduce allocations for path generation and manipulation we need Span overloads for System.IO.Path APIs.

Proposed API

namespace System.IO
{
    public static class Path
    {
        public static ReadOnlySpan<char> GetDirectoryName(ReadOnlySpan<char> path);
        public static ReadOnlySpan<char> GetExtension(ReadOnlySpan<char> path);
        public static ReadOnlySpan<char> GetFileName(ReadOnlySpan<char> path);
        public static ReadOnlySpan<char> GetFileNameWithoutExtension(ReadOnlySpan<char> path);
        public static ReadOnlySpan<char> GetPathRoot(ReadOnlySpan<char> path);
        public static bool HasExtension(ReadOnlySpan<char> path);
        public static bool IsPathFullyQualified(ReadOnlySpan<char> path);
        public static bool IsPathRooted(ReadOnlySpan<char> path);
    }
}

Implementation Notes

  • If the current APIs return null, these would return an empty span.
@JeremyKuhne JeremyKuhne self-assigned this Nov 28, 2017
@karelz
Copy link
Member

karelz commented Nov 28, 2017

FYI: The API review discussion was recorded - see https://youtu.be/9F_NsviGT0Y?t=552 (7 min duration)

@JeremyKuhne
Copy link
Member Author

This change is pretty straight-forward. We'll need to clean up internal "index of" helpers and have existing APIs call these new public methods. Tests will need to be added.

@danmoseley
Copy link
Member

5 days assuming it's someone less familar with the area, including cleanup mentioned.

@danmoseley danmoseley assigned Anipik and unassigned JeremyKuhne Dec 14, 2017
@Anipik Anipik closed this as completed Jan 31, 2018
@dsyme
Copy link

dsyme commented Apr 19, 2018

Just to mention I don't like these changes. I think they make these very basic APIs considerably harder to use for beginners.

Really. we shouldn't be littering Span<T> around everywhere in overloads of the long-standing, simple parts of the .NET APIs just because it's the new cool thing. It's a performance type that's completely unnecessary to know about for 98% of users or more.

I trust basic usability of .NET for very basic file system operations beginners stays on the radar of the people doing the API reviews.

p.s. Adding overloads can also break type inference in F# code, but that's a separate matter.

@danmoseley
Copy link
Member

@dsyme assuming some folks do need these for perf (like ourselves) are you suggesting they should have gone on a new type? (PathSpan..)

@jkotas Could tooling someday help here, somehow hiding overloads that differ only in ReadOnlySpan vs string, and just letting the compiler bind to the right one? (That doesn't help where the signature changes entirely eg the Try pattern ones)

@JeremyKuhne
Copy link
Member Author

Just to mention I don't like these changes. I think they make these very basic APIs considerably harder to use for beginners.

These weren't added for the coolness factor. These overloads are very important for I/O heavy solutions. Allocations for path manipulation is a significant drag on VS/MSBuild for example.

They also aren't intended for beginners. The existing APIs are still there and can and should be used by people who don't care about allocations. We could, in theory, move them into a different class or namespace, but I'm afraid that boat has already sailed. Discoverability was judged to be more important for APIs that match existing ones- and that has been done throughout the framework.

@jkotas
Copy link
Member

jkotas commented Apr 19, 2018

@jkotas Could tooling someday help here, somehow hiding overloads that differ only in ReadOnlySpan vs string

You are typing Path.. What is the crystal ball IntelliSense should use to decide whether to show you the expert Span overloads; or just the regular string overloads?

I guess there can be AI applied to it, classify the users and the APIs, and then decide that most of the users like you do not use Span Path APIs and do not show you those either. I am not saying it is a good idea ... just brainstorming.

BTW: As some of you know I am worried that there are usability issues with the new Path APIs even for expert use. dotnet/coreclr#17429 (comment) and dotnet/coreclr#17528 (comment) has some of the discussions.

@danmoseley
Copy link
Member

If we could show a notional IString then perhaps we could collapse the two since most people would assume that IString would accept a String. I think of ReadOnlySpan<char> as just an extension of string (or can be). Yeah, I guess this doesn't make sense and I don't have an answer.

@JeremyKuhne
Copy link
Member Author

BTW: As some of you know I am worried that there are usability issues with the new Path APIs even for expert use. dotnet/coreclr#17429 (comment) and dotnet/coreclr#17528 (comment) has some of the discussions.

Note that the linked issues are APIs that haven't shipped and are very different from these ones, which are complete and part of 2.1. We're still trying to navigate the right pattern for APIs that take an output buffer.

@msftgits msftgits transferred this issue from dotnet/corefx Jan 31, 2020
@msftgits msftgits added this to the 2.1.0 milestone Jan 31, 2020
@dotnet dotnet locked as resolved and limited conversation to collaborators Dec 19, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
api-approved API was approved in API review, it can be implemented area-System.IO
Projects
None yet
Development

No branches or pull requests

7 participants