NightOwl888 commented on code in PR #801:
URL: https://github.com/apache/lucenenet/pull/801#discussion_r1161159514
##########
src/Lucene.Net.QueryParser/Flexible/Standard/Nodes/NumericRangeQueryNode.cs:
##########
@@ -87,7 +87,9 @@ private static NumericType
GetNumericDataType(J2N.Numerics.Number number)
/// <param name="lowerInclusive"><c>true</c> if the lower bound is
inclusive, otherwise, <c>false</c></param>
/// <param name="upperInclusive"><c>true</c> if the upper bound is
inclusive, otherwise, <c>false</c></param>
/// <param name="numericConfig">the <see cref="Config.NumericConfig"/>
that represents associated with the upper and lower bounds</param>
- public virtual void SetBounds(NumericQueryNode lower, NumericQueryNode
upper,
+ // LUCENENET specific - S1699 - marked non-virtual because calling
+ // virtual members from the constructor is not a safe operation in .NET
+ public void SetBounds(NumericQueryNode lower, NumericQueryNode upper,
Review Comment:
This one can be replaced by a private `SetBoundsInternal()` method called by
the constructor and public virtual `SetBounds()`.
BTW - Let's add some documentation on `SetBounds()` letting users know that
if they override the method they would also need to add the same logic into the
constructor to match Lucene.
##########
src/Lucene.Net/Util/SentinelIntSet.cs:
##########
@@ -89,7 +89,9 @@ public SentinelInt32Set(int size, int emptyVal)
}
[MethodImpl(MethodImplOptions.AggressiveInlining)]
- public virtual void Clear()
+ // LUCENENET specific - S1699 - marked non-virtual because calling
+ // virtual members from the constructor is not a safe operation in .NET
+ public void Clear()
Review Comment:
BTW - Let's add some documentation on `Clear()` letting users know that if
they override the method they would also need to add the same logic into the
constructor to match Lucene.
##########
src/Lucene.Net/Search/Sort.cs:
##########
@@ -139,13 +139,17 @@ public Sort(params SortField[] fields)
}
/// <summary>Sets the sort to the given criteria. </summary>
- public virtual void SetSort(SortField field)
+ // LUCENENET specific - S1699 - marked non-virtual because calling
+ // virtual members from the constructor is not a safe operation in .NET
+ public void SetSort(SortField field)
{
Review Comment:
These can be made into `SetSortInternal()` called by the constructor and
virtual `SetSort()`.
BTW - Let's add some documentation on both overloads of `SetSort()` letting
users know that if they override the method they would also need to add the
same logic into the constructor to match Lucene.
##########
src/Lucene.Net/Search/IndexSearcher.cs:
##########
@@ -161,7 +161,9 @@ public IndexSearcher(IndexReaderContext context)
/// Each <see cref="LeafSlice"/> is executed in a single thread. By
default there
/// will be one <see cref="LeafSlice"/> per leaf (<see
cref="AtomicReaderContext"/>).
/// </summary>
- protected virtual LeafSlice[] Slices(IList<AtomicReaderContext> leaves)
+ // LUCENENET specific - S1699 - marked non-virtual because calling
+ // virtual members from the constructor is not a safe operation in .NET
+ protected LeafSlice[] Slices(IList<AtomicReaderContext> leaves)
Review Comment:
This one can be made into `GetSlicesInternal()` called by the constructor
and virtual `GetSlices()` method. Note adding a verb here is intentional to
indicate this is an action. And since it is protected, this break won't affect
many users. Just be sure to add a note that we renamed this from Lucene.
BTW - Let's add some documentation on `GetSlices()` letting users know that
if they override the method they would also need to add the same logic into the
constructor to match Lucene.
##########
src/Lucene.Net/Search/DisjunctionMaxQuery.cs:
##########
@@ -92,7 +92,9 @@ public DisjunctionMaxQuery(ICollection<Query> disjuncts,
float tieBreakerMultipl
/// <summary>
/// Add a subquery to this disjunction </summary>
/// <param name="query"> The disjunct added </param>
- public virtual void Add(Query query)
+ // LUCENENET specific - S1699 - marked non-virtual because calling
+ // virtual members from the constructor is not a safe operation in .NET
+ public void Add(Query query)
Review Comment:
BTW - Let's add some documentation on `Add()` letting users know that if
they override the method they would also need to add the same logic into the
constructor to match Lucene.
##########
src/Lucene.Net/Store/ByteArrayDataOutput.cs:
##########
@@ -51,12 +51,16 @@ public ByteArrayDataOutput()
Reset(BytesRef.EMPTY_BYTES);
}
- public virtual void Reset(byte[] bytes)
+ // LUCENENET specific - S1699 - marked non-virtual because calling
+ // virtual members from the constructor is not a safe operation in .NET
+ public void Reset(byte[] bytes)
Review Comment:
This can be changed to `ResetInternal()` called by both the constructor and
the virtual `Reset()` methods.
BTW - Let's add some documentation on `Reset()` letting users know that if
they override the method they would also need to add the same logic into the
constructor to match Lucene.
Also, let's add some null guard clauses for `bytes` that throw
`ArgumentNullException`.
##########
src/Lucene.Net.Spatial/Prefix/Tree/Cell.cs:
##########
@@ -175,7 +175,9 @@ private void B_fixLeaf()
public virtual bool IsLeaf => m_leaf;
/// <summary>Note: not supported at level 0.</summary>
- public virtual void SetLeaf()
+ // LUCENENET specific - S1699 - marked non-virtual because calling
+ // virtual members from the constructor is not a safe operation in .NET
+ public void SetLeaf()
Review Comment:
BTW - Let's add some documentation on `SetLeaf()` letting users know that if
they override the method they would also need to add the same logic into the
constructor to match Lucene.
--
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
