NightOwl888 commented on code in PR #1056:
URL: https://github.com/apache/lucenenet/pull/1056#discussion_r1882041412
##########
src/Lucene.Net.Benchmark/Quality/QualityQuery.cs:
##########
@@ -82,22 +82,59 @@ public virtual string GetValue(string name)
/// For a nicer sort of input queries before running them.
/// Try first as ints, fall back to string if not int.
/// </summary>
- /// <param name="other"></param>
- /// <returns></returns>
- public virtual int CompareTo(QualityQuery other)
+ /// <param name="other">The other <see cref="QualityQuery"/> to
compare to.</param>
+ /// <returns>0 if equal, a negative value if smaller, a positive value
if larger.</returns>
+ public virtual int CompareTo(QualityQuery? other)
{
- try
+ if (other is null)
{
- // compare as ints when ids ints
- int n = int.Parse(queryID, CultureInfo.InvariantCulture);
- int nOther = int.Parse(other.queryID,
CultureInfo.InvariantCulture);
- return n - nOther;
+ return 1;
}
- catch (Exception e) when (e.IsNumberFormatException())
+
+ if (int.TryParse(queryID, NumberStyles.Integer,
CultureInfo.InvariantCulture, out int n)
+ && int.TryParse(other.queryID, NumberStyles.Integer,
CultureInfo.InvariantCulture, out int nOther))
{
- // fall back to string comparison
- return queryID.CompareToOrdinal(other.queryID);
+ return n - nOther;
}
+
+ // fall back to string comparison
+ return queryID.CompareToOrdinal(other.queryID);
+ }
+
+ // LUCENENET specific - provide Equals and GetHashCode due to
providing operator overrides
+ protected bool Equals(QualityQuery? other) => queryID ==
other?.queryID;
Review Comment:
Being that `IEquatable<T>` is not being implemented (and we don't need to
because this is a reference type), this extra overload doesn't seem necessary.
Please move this into the `public override bool Equals(object? obj)` method and
remove this one.
##########
src/Lucene.Net.QueryParser/Surround/Query/SimpleTerm.cs:
##########
@@ -28,8 +28,8 @@ namespace Lucene.Net.QueryParsers.Surround.Query
public abstract class SimpleTerm : SrndQuery, IDistanceSubQuery,
IComparable<SimpleTerm>
Review Comment:
Being that `CompareTo()` is deperecated in this class, let's suppress the
warnings for overriding `Equals(object)` and `GetHashCode()`.
##########
src/Lucene.Net.Suggest/Suggest/Fst/FSTCompletion.cs:
##########
@@ -67,6 +67,29 @@ public int CompareTo(Completion o)
{
return this.Utf8.CompareTo(o.Utf8);
Review Comment:
Please override `Equals(object)` and `GetHashCode()` to suppress the
warnings.
I think the most logical thing to do here is to cascade the call to the base
class and continue using reference equality.
> We have introduced bugs in the past by changing from reference equality to
equality based on fields (even though the `GetHashCode()` method was based on
fields), so we would need to test it thoroughly if we change it.
##########
src/Lucene.Net.Suggest/Suggest/Lookup.cs:
##########
@@ -122,14 +122,37 @@ public int CompareTo(LookupResult o)
{
return CHARSEQUENCE_COMPARER.Compare(Key, o.Key);
Review Comment:
Please override `Equals(object)` and `GetHashCode()` to suppress the
warnings.
I think the most logical thing to do here is to continue using reference
equality by cascading the calls to the base class.
--
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: dev-unsubscr...@lucenenet.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
