NightOwl888 commented on code in PR #982:
URL: https://github.com/apache/lucenenet/pull/982#discussion_r1810799106
##########
src/Lucene.Net/Support/Collections.cs:
##########
@@ -213,6 +213,21 @@ public static string ToString(object obj, CultureInfo
culture)
return ToString(obj);
}
+ public static ReadOnlyList<T> AsReadOnly<T>(IList<T> list)
+ {
+ return new ReadOnlyList<T>(list);
+ }
+
+ public static ReadOnlyCollection<T> AsReadOnly<T>(ICollection<T>
collection)
Review Comment:
A few things to note here:
1. This method has no callers according to code lens, so YAGNI.
2. In Java, Collections have undefined behavior, but Set, List, and Map all
have a specific way they perform structural equality comparisons and do
structural formatting. So, it would be best to avoid using
`ReadOnlyCollection<T>`, since it has to do extra checks to work out if we are
dealing with a `Set`, `List`, or `Dictionary` type before cascading the call to
the code that knows how to deal with them, and fallback to a generic
implementation if none of them match. So, if we are going for a complete set of
collection methods, then `ReadOnlySet<T>` should be the choice here, not
`ReadOnlyCollection<T>`.
3. Each of the read only collections in J2N has a "Default" mode, which
kicks in if the type being passed in is a J2N collection, where the rules of
structural equality are defined. If it is a BCL or 3rd party collection, it
will use "Aggressive" mode, which tries to patch the collection externally with
the same behavior. But "Aggressive" mode is very costly in terms of performance.
Given number 3 above, we should only use "Aggressive" mode if we are dealing
with a user-supplied collection that we have no control over. However, if we
have control over the concrete collection type that is used (or default), we
should always ensure that a J2N collection is used for each of these calls so
we don't downgrade to "Aggressive" mode. So, it would be a good idea to review
to make sure we didn't swap to any BCL collections where it could matter.
It would probably also be sensible to add warnings to our Roslyn code
analyzer to warn users that there may be a performance downgrade if they use a
BCL collection type in cases where there would be a performance downgrade. This
only happens when comparing collection equality or formatting the contents of
the collection, IIRC.
We probably should make the above 2 paragraphs into new issues.
--
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]
