IndexSearcher fix for virtual calls from constructor #837
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
NightOwl888
requested changes
Apr 14, 2023
| /// control whether or not the leaf slices are allocated. | ||
| /// If you choose to skip allocating the leaf slices here, you must | ||
| /// initialize <see cref="m_leafSlices" /> in your subclass's constructor, either | ||
| /// by calling <see cref="GetSlices"/> or using your own logic |
There was a problem hiding this comment.
Let's be a bit more specific:
/// <summary>
/// LUCENENET specific constructor that can be used by the subclasses to
/// control whether the leaf slices are allocated in the base class or subclass.
/// </summary>
/// <remarks>
/// If executor is non-<c>null</c> and you choose to skip allocating the leaf slices
/// (i.e. <paramref name="allocateLeafSlices"/> == <c>false</c>), you must
/// set the <see cref="m_leafSlices"/> field in your subclass constructor.
/// This is commonly done by calling <see cref="GetSlices(IList{AtomicReaderContext})"/>
/// and using the result to set <see cref="m_leafSlices"/>. You may wish to do this if you
/// have state to pass into your constructor and need to set it prior to the call to
/// <see cref="GetSlices(IList{AtomicReaderContext})"/> so it is available for use
/// as a member field or property inside a custom override of
/// <see cref="GetSlices(IList{AtomicReaderContext})"/>.
/// </remarks>
| @@ -464,6 +471,9 @@ protected virtual TopDocs Search(Weight weight, ScoreDoc after, int nDocs) | |||
| ReentrantLock @lock = new ReentrantLock(); | |||
| ExecutionHelper<TopDocs> runner = new ExecutionHelper<TopDocs>(executor); | |||
|
|
|||
| if (m_leafSlices == null) | |||
There was a problem hiding this comment.
Although it doesn't make any difference for arrays, all null equality checks have all been converted to use is in #617.
The is keyword doesn't take any overrides of the == operator into account, so it is not possible to circumvent our null check logic by passing a class that has one.
| @@ -560,6 +570,9 @@ protected virtual TopFieldDocs Search(Weight weight, FieldDoc after, int nDocs, | |||
| ReentrantLock @lock = new ReentrantLock(); | |||
| ExecutionHelper<TopFieldDocs> runner = new ExecutionHelper<TopFieldDocs>(executor); | |||
|
|
|||
| if (m_leafSlices == null) | |||
There was a problem hiding this comment.
See my comment on line 474.
| @@ -464,6 +471,9 @@ protected virtual TopDocs Search(Weight weight, ScoreDoc after, int nDocs) | |||
| ReentrantLock @lock = new ReentrantLock(); | |||
| ExecutionHelper<TopDocs> runner = new ExecutionHelper<TopDocs>(executor); | |||
|
|
|||
| if (m_leafSlices == null) | |||
| throw new InvalidOperationException("m_leafSlices must not be null and initialized in the constructor"); | |||
There was a problem hiding this comment.
Let's change this a bit:
throw new InvalidOperationException($"When the constructor is passed a non-null {nameof(TaskScheduler)}, {nameof(m_leafSlices)} must also be set to a non-null value in the constructor.");| @@ -560,6 +570,9 @@ protected virtual TopFieldDocs Search(Weight weight, FieldDoc after, int nDocs, | |||
| ReentrantLock @lock = new ReentrantLock(); | |||
| ExecutionHelper<TopFieldDocs> runner = new ExecutionHelper<TopFieldDocs>(executor); | |||
|
|
|||
| if (m_leafSlices == null) | |||
| throw new InvalidOperationException("m_leafSlices must not be null and initialized in the constructor"); | |||
There was a problem hiding this comment.
Let's change this a bit:
throw new InvalidOperationException($"When the constructor is passed a non-null {nameof(TaskScheduler)}, {nameof(m_leafSlices)} must also be set to a non-null value in the constructor.");
NightOwl888
requested changes
Apr 14, 2023
| /// control whether the leaf slices are allocated in the base class or subclass. | ||
| /// </summary> | ||
| /// <remarks> | ||
| /// If executor is non-<c>null</c> and you choose to skip allocating the leaf slices |
There was a problem hiding this comment.
Oops. I missed that the second word here should be <paramref name="executor"/>.
NightOwl888
requested changes
Apr 14, 2023
…s for all non-nullable parameters. Marked nullable parameters as such. Documented when ArgumentNullReferenceException is expected to be thrown from each method overload.
… StoredFieldVisitor)
NightOwl888
approved these changes
Apr 14, 2023
laimis
commented
Apr 16, 2023
| // LUCENENET: Added guard clause | ||
| if (leaves is null) | ||
| throw new ArgumentNullException(nameof(leaves)); | ||
|
|
||
| LeafSlice[] slices = new LeafSlice[leaves.Count]; |
There was a problem hiding this comment.
I am not a fan of these type of additions. This is not present in the original source, not even in the latest version. We could add a lot of extra typing/code (that's not even interesting, just copy paste of exception docs and guar checks). This is why I wasn't sure what you meant by guard clauses as it seemed excessive.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Continuation of fixes with virtual calls being made from constructors. The issue originally reported by SonarCloud scans: https://sonarcloud.io/project/issues?resolved=false&rules=csharpsquid%3AS1699&id=apache_lucenenet and referenced in this issue: #670
This one focuses on IndexSearcher. Since we discovered a separate constructor approach as an elegant way to address this issue, let's use it for IndexSearcher.