NightOwl888 commented on code in PR #819:
URL: https://github.com/apache/lucenenet/pull/819#discussion_r1162973109
##########
src/Lucene.Net/Codecs/BlockTreeTermsReader.cs:
##########
@@ -114,10 +114,14 @@ public class BlockTreeTermsReader : FieldsProducer
private readonly int version;
+ protected readonly object subclassState;
Review Comment:
For protected members, we are consistently using the convention of using a
`m_` prefix.
This is because there are several places in Lucene where there are public
setters and getters for a protected field. For example, where there would be
1. A protected field `length`
2. A public virtual method named `getLength()`
3. A public virtual method named `setLength()`
It would be converted as
1. A protected field `m_length`
2. A public property get/set named `Length`
The issue is that is that if we didn't rename the protected field, we would
have non-CLS compliant code, since case is not enough to distinguish member
names. Unfortunately, the most common way of specifying fields `_length` is
also not CLS compliant. So, we went with another common convention `m_` which
is CLS compliant.
While we technically don't need to name all protected members this way, we
did it across the codebase for consistency.
##########
src/Lucene.Net/Codecs/BlockTreeTermsReader.cs:
##########
@@ -114,10 +114,14 @@ public class BlockTreeTermsReader : FieldsProducer
private readonly int version;
+ protected readonly object subclassState;
+
/// <summary>
/// Sole constructor. </summary>
- public BlockTreeTermsReader(Directory dir, FieldInfos fieldInfos,
SegmentInfo info, PostingsReaderBase postingsReader, IOContext ioContext,
string segmentSuffix, int indexDivisor)
+ public BlockTreeTermsReader(Directory dir, FieldInfos fieldInfos,
SegmentInfo info, PostingsReaderBase postingsReader, IOContext ioContext,
string segmentSuffix, int indexDivisor, object subclassState = default)
Review Comment:
Maybe instead of making this optional, it should be required and all callers
should pass `null`.
See the Slack thread about another idea of making these 2 classes generic.
Either way, we are breaking the API, but at least using a generic, there would
be a way to pass state without having to resort to always using a reference
type (which may involve boxing).
These classes do not exist in the latest version of Lucene, so there are no
issues with upgrading if it were made generic.
##########
src/Lucene.Net/Codecs/BlockTreeTermsReader.cs:
##########
@@ -114,10 +114,14 @@ public class BlockTreeTermsReader : FieldsProducer
private readonly int version;
+ protected readonly object subclassState;
+
/// <summary>
/// Sole constructor. </summary>
- public BlockTreeTermsReader(Directory dir, FieldInfos fieldInfos,
SegmentInfo info, PostingsReaderBase postingsReader, IOContext ioContext,
string segmentSuffix, int indexDivisor)
+ public BlockTreeTermsReader(Directory dir, FieldInfos fieldInfos,
SegmentInfo info, PostingsReaderBase postingsReader, IOContext ioContext,
string segmentSuffix, int indexDivisor, object subclassState = default)
Review Comment:
Also, it looks like we need to add this parameter to the XML docs for
`Lucene40PostingsFormat` and `Lucene41PostingsFormat` to avoid lots of warnings.
--
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
