& needs to be escaped in xml as & (applies throughout these xml doc comments)

Changing

Any reason this couldn't implement both IEnumerable<T> and IEnumerator<T>, and return itself from GetEnumerator in the common case to avoid the enumerator allocation, like Win32FileSystemEnumerableIterator<TSource> before this change?

I was thinking of being friendly to Linq queries being run multiple times. I'll think it through some more.

It should still support being able to call GetEnumerator multiple times... in such cases, you'd create a new instance with initial state.

It should still support being able to call GetEnumerator multiple times... in such cases, you'd create a new instance with initial state.

Not sure I follow. How would that save an allocation?

Take a look at Iterator<TSource> (which is the base class of Win32FileSystemEnumerableIterator<TSource>) :

Iterator<TSource> implements both IEnumerable<T> and IEnumerator<T>.

Here's the GetEnumerator implementation:

The common case when using foreach or a LINQ query is to get the enumerable and then immediately call GetEnumerator to get the enumerator to start enumerating through the enumerable (e.g. foreach (string file in Directory.EnumerateFiles(...)) { ... }). If GetEnumerator hasn't already been called and it's the same thread, it can simply return itself. Otherwise, a new instance is created.

This is also how iterators work in C# when you use yield return. Similar iterator code is emitted by the compiler to use the same instance for both the enumerable and enumerator, only allocating a new instance if GetEnumerator if necessary.

Fair enough- I'll look at utilizing Iterator.

Whether or not Iterator is used as a base class, the same approach of reusing the enumerable object as the enumerator object for at least the initial enumeration should definitely be done.

Making the change now.

Any reason these can't use the recently added C# stackalloc/span support, e.g.

Span<int> priorMatches = stackalloc int[16];
Span<int> currentMatches = stackalloc int[16];

at which point, it looks like this method would no longer need to be unsafe.

Wasn't aware of it. Tried switching and it wouldn't let me do the swap between the two anymore (at the end of the method).

Tried switching and it wouldn't let me do the swap between the two anymore (at the end of the method).

@JeremyKuhne, what error do you get?
@VSadov?

I realize this is nested within an internal type, so it will never actually be public, but I'm curious: should these nested types be public or internal? The visibility of Interop types in this PR are inconsistent. Is there an existing convention?

Internal is better- I'll fix it

Thanks

Was this intended to remain?

No :) Thanks!

Can any of these fields be readonly?

I'll double check.

Passing FindTransforms.AsUserFullPath and predicate like this is going to result in two delegate allocations each time UserFiles is called. Same for the methods below. It'd be nice if these delegate instances were lazily allocated and cached/reused.

You could attempt to address this by writing it as:

    	FindTransform<string> transform = (ref RawFindData findData) =>
        {
            ReadOnlySpan<char> subdirectory = findData.Directory.AsReadOnlySpan().Slice(findData.OriginalDirectory.Length);
            return PathHelpers.CombineNoChecks(findData.OriginalUserDirectory, subdirectory, findData.FileName);
        };
        
        FindPredicate predicate = (ref RawFindData findData) =>
        {
            return FindPredicates.NotDotOrDotDot(ref findData)
                && !FindPredicates.IsDirectory(ref findData)
                && DosMatcher.MatchPattern(expression, findData.FileName, ignoreCase: true);
        };
    
    	return new FindEnumerable<string>(directory, recursive, transform, predicate);

The C# compiler will lazily allocate and cache/reuse the transform delegate, but predicate is problematic as written above as it captures expression resulting in a closure allocation.

To avoid that, you'd have to change the FindPredicate signature to also take a string expression parameter, and then pass the expression to the FindEnumerable constructor so it can pass it along whenever it calls predicate, e.g.:

internal delegate bool FindPredicate(string expression, ref RawFindData findData);

        FindPredicate predicate = (string expr, ref RawFindData findData) =>
        {
            return FindPredicates.NotDotOrDotDot(ref findData)
                && !FindPredicates.IsDirectory(ref findData)
                && DosMatcher.MatchPattern(expr, findData.FileName, ignoreCase: true);
        };

        return new FindEnumerable<string>(directory, expression, recursive, transform, predicate);

        _predicate(_expression, ref findData);

I can do the first part for sure. Redefining the predicate delegate is problematic I think. Not all predicates need the extra parameter and future ones will need different arguments (think all files modified after some DateTime).

Not all predicates need the extra parameter and future ones will need different arguments

Can FindPredicate take a generic TState that can be used to pass any extra info needed?

Alternatively, instead of using delegates for the predicate and transform you could have abstract/virtual Transform/Predicate methods on the enumerable class, and have sealed subclasses for each type of enumeration.

Is it OK to call CloseHandle multiple times if Dispose is called multiple times?

I'll move it up to the buffer interlock to be safe about it, thanks!

string result is unnecessary. (multiple places)

Thanks- missed taking this out. Debugging through the AsReadOnlySpan() kept randomly terminating the debugger (under the latest public release of VS 2017) so I temporarily put this in. @stephentoub, are you aware of any issues like this with debugging Span? I'm not sure where to begin looking for more details when the debugger terminates unexpectedly...

I would think that

    internal enum BOOLEAN : byte
    {
        FALSE = 0,
        TRUE = 1,
    }

should be sufficient given the limited use. Similar how Interop.BOOL.cs is done.

The definition I have deals with non 0/1 values can be used just like a bool. What do we gain by making it an enum other than a simpler definition?

You will get a simpler definition that was my point.

Do you plan to address this? We are inconsistent today - both Interop.BOOL.cs and Interop.BOOLEAN.cs should use the same style.

Do you plan to address this? We are inconsistent today - both Interop.BOOL.cs and Interop.BOOLEAN.cs should use the same style.

As both you and Stephen like the enum version I'll switch it to that format.

Sight...
I am wondering whether the new string stringCreate<TState>(int length, TState state, System.Buffers.SpanAction<char, TState> action) API is good since you cannot really use it here.
cc @stephentoub

since you cannot really use it here

Because you can't pass spans in generics? We're going to have that problem in a bunch of places and is something we should try to solve more generally, e.g. a "ref struct" constraint and then an overload that accepts a delegate that takes such a constrained arg (and maybe have a built-in value tuple constrained to ref structs).

You can use the overload here, though, it just means pinning the input spans instead of the destination string, e.g.

using (char* firstPtr = &first.DangerousGetPinnableReference(), secondPtr = &second.DangerousGetPinnableReference())
{
    return string.Create(totalLength, ((IntPtr)firstPtr, first.Length, (IntPtr)secondPtr, second.Length, hasSeparator), (dst, state) =>
    {
        new ReadOnlySpan<char>((char*)state.Item1, state.Item2).CopyTo(dst);
        if (!state.Item5) dst[state.Item2] = Path.DirectorySeparatorChar;
        new ReadOnlyspan<char>((char*)state.Item3, state.Item4).CopyTo(dst.Slice(state.Item2 + (state.Item5 ? 0 : 1));
    });
}

etc. And tuple arg names could be used to avoid those ItemXs.

I'll try using string.Create

Instead of adding AggressiveInlining inlining that leads to codebloat, you should avoid the need for marshaling in CompareStringOrdinal (use char* and BOOL in the signature) and do the marshaling yourself (using fixed, etc.). It will be smaller and faster code that way.

I'm not sure I follow exactly. Do fully blittable arguments (when they're simple types) make that much of a difference in imports? We've been encouraging using ref char in other places to avoid needing to use fixed... @stephentoub I recall you suggesting ref char for Span- I might be remembering incorrectly. I want to make sure we're clear on guidance here.

The results and error handling aren't exactly straight forward so I think having a wrapper method for usage is appropriate. (I also think this is a method we'd want to ultimately have public). I'm fine with removing the AggressiveInlining if the advantage is trivial.

I've encouraged using ref instead of pointers to keep the code simpler and minimize unsafe pointer usage that we could mess up. If specific cases benefit from using pointers instead for perf, that's fine.

If it's equivalent to FILE_FULL_DIR_INFO, shouldn't it be called FILE_FULL_DIR_INFO rather than FILE_FULL_DIR_INFORMATION?

The same struct is defined twice in Windows under those two names. Given that I had to pick one I went with the "real" one.

Then shouldn't the comment match? If there are two equally valid names, it's fine to pick one, but we should pick one ;)

It does- the summary calls out the actual name we used first. I also list the second definition as it isn't inherently obvious and we use it in both Win32 and NT API imports.

Fine

It's ok that this is being exempted?

Not sure- I called it out in my PR comments. I'm following up. If I have to I'll add logic to fall back to GetFileInformationByHandleEx when it isn't available. Win7 doesn't have the support we need in said API and it is slightly less efficient so we'd prefer to use the NtQuery API instead. It is sort of strange that they might not expose the ability to convert NTSTATUS to windows errors, given that they expose NTSTATUS. I'm trying to confirm that this API doesn't exist or if there is another entry point that gets to the same underlying API.

This API is legalized for UWP (and it's not a new API) so it should be fine to use it on Nano, etc: our OneCore baseline just isn't updated to include it. I've reached out to understand exactly. This exclusion is fine.

Thanks for looking into it!

I don't understand the logic here. Shouldn't this be if (!ignoreCase)?

Yes, my mistake.

also ==true and ==false are ugly in general.

@KrzysztofCwalina, @ahsonkhan, another case where we're having to write this logic in multiple places, e.g. here and https://github.com/dotnet/coreclr/blob/master/src/mscorlib/shared/System/StringSpanHelpers.cs#L40. Relates to https://github.com/dotnet/corefx/issues/21395.

Yeah, it's totally in the plan to add such APIs. We just need to find some cycles to do it.

Thanks. Just wanted to highlight the proliferation :)

Do we need the first part of the condition here? Couldn't it just be:

else if (value.Length > span.Length)

?

Good catch, thanks

How about storing fullPath ?? originalPath into a local? It's used three times in the above three lines.

Ok

Why are we using unsafe code here? I would expect the JIT would do a very good job with:

foreach (char c in expression)

or

for (int i = 0; i < expression.Length; i++)
{
    char c = expression[i];
    ...
}

Fair enough- I'll trust the JIT :)

Nit: extra space between single and starting