BREAKING: remove virtual call from the constructor for IndexSearcher

[laimis]

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. Creating a private method that constructor can call and calling the private method from the virtual method.

[NightOwl888]

Oops, looks like this wasn't made private.

But after taking a fresh look at this, I see that the m_leafSlices field the constructor sets is already protected. A subclass would simply need to pass null for executor and they would be able to call m_leafSlices.GetSlices(context. Leaves) themselves.

The only limitation is that they wouldn't be able to set executor because it is marked private, so it wouldn't go down the execution path that actually uses m_leafSlices.

The current solution calls GetSlicesInternal() and sets m_leafSlices, which is an array allocation. The subclass has no control over this. Then, if they wanted to set state before it is called, the only choice is to allocate this array again by calling GetSlices() from the subclass constructor. So, not only would it need to be called twice, the overridden GetSlices() method would need to account for this.

I think a better solution would be to add a protected constructor with the ability to opt out of the GetSlices() call. Then they can set any state passed in from their constructor prior to calling GetSlices() and there is only one allocation.

protected IndexSearcher(IndexReaderContext context, TaskScheduler executor, bool allocateLeafSlices)

Of course, we need to make it very clear that when passing false for allocateLeafSlices they will need to do it in their constructor. It would also be good to put guard clauses on all usages of m_leafSlices so that if it is null during search the user should get an InvalidOperationException explaining that m_leafSlices must be set in the constructor (and it must be there since it is readonly). Of course, it would also be helpful to mention they can set it by calling the following in the constructor documentation.

this.m_leafSlices = GetSlices(m_leafContexts);

Given that this is one of the main APIs of this library, I think it is worth the diligence to get it right. If you can come up with a more elegant solution than this, feel free to suggest it.

[laimis]

Great idea, made a new PR: #837