paulirwin commented on code in PR #1058:
URL: https://github.com/apache/lucenenet/pull/1058#discussion_r1997463102
##########
src/Lucene.Net.Memory/MemoryIndex.cs:
##########
@@ -275,23 +284,45 @@ public virtual TokenStream
KeywordTokenStream<T>(ICollection<T> keywords)
return new TokenStreamAnonymousClass<T>(keywords);
}
+ /// <summary>
+ /// An anonymous implementation of <see cref="TokenStream"/> for
+ /// <see cref="KeywordTokenStream{T}(ICollection{T})"/>.
+ /// </summary>
+ /// <typeparam name="T">The type of item in the collection.</typeparam>
+ /// <remarks>
+ /// LUCENENET specific - This class originally got an enumerator in
the constructor and stored it to a field
+ /// that was never reset, which meant that it could not be reused
(since most IEnumerator implementations can
+ /// only be iterated once and throw on <see
cref="System.Collections.IEnumerator.Reset()"/>). This class has
Review Comment:
First, I should have worded this differently; I should have said "many"
instead of "most." I meant to communicate that we generally can't rely on using
it in Reset, because many implementations do throw. For example, any
enumerators created by LINQ or the Enumerable class throw on Reset, which are
pretty commonly used. Note that [Microsoft's own docs on
IEnumerator](https://learn.microsoft.com/en-us/dotnet/api/system.collections.ienumerator?view=net-9.0#remarks)
say that Reset "is provided for COM interoperability and does not need to be
fully implemented; instead, the implementer can throw a
[NotSupportedException](https://learn.microsoft.com/en-us/dotnet/api/system.notsupportedexception?view=net-9.0)."
I think it's likely enough that it will throw that alternative 1 is not a good
idea.
Alternative 3 seems like it would be more costly than just allowing it to
possibly allocate on reset. Enumerators for BCL types are highly optimized
nowadays, and for some of the common ones you'd at most have a boxed struct. It
seems to me like having try/catch unwinding would be more costly in runtime
performance.
Alternative 2 is fine, if you really think we need it. I don't think we do.
It is quite common to get and dispose of an enumerator in .NET. There are over
2,000 uses of `foreach` in our codebase which get a new enumerator each time
that I doubt we are applying such scrutiny to. But, if you think it's
important, I'd choose this option.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
