mawiesne commented on code in PR #1161:
URL: https://github.com/apache/opennlp/pull/1161#discussion_r3607478125
##########
opennlp-api/src/main/java/opennlp/tools/util/normalizer/Alignment.java:
##########
@@ -147,22 +147,21 @@ public Alignment andThen(Alignment next) {
final int start = middleStart < normalizedLength() ?
originalStart[middleStart] : originalLength;
final int end = middleEnd > 0 ? originalEnd[middleEnd - 1] : 0;
starts[f] = start;
- // Math.max keeps the original span non-inverted. When next inserted
this final character
- // (a zero-width middle range, middleStart == middleEnd) the max
collapses it to a zero-width
- // original span -- correct for every insertion except one landing
strictly inside an
- // expansion this stage produced, where the characters on either side
share one atomic
- // original block (originalEnd[middleEnd - 1] >
originalStart[middleStart]) that has no
- // interior offset to point at. There the insertion is attributed to
that whole block, the
- // only choice that keeps originalStart/originalEnd sorted so
toOriginalSpan/toNormalizedSpan
- // keep their O(log n) search; forcing it to zero-width would push
originalEnd below its
- // predecessor and corrupt the reverse mapping.
+ // Math.max keeps the original span non-inverted and keeps
originalStart/originalEnd sorted so
Review Comment:
Please keep the previous, more detailed code comment.
##########
opennlp-core/opennlp-runtime/src/main/java/opennlp/tools/util/normalizer/DigitCharSequenceNormalizer.java:
##########
@@ -32,22 +32,35 @@ public class DigitCharSequenceNormalizer implements
OffsetAwareNormalizer {
private static final DigitCharSequenceNormalizer INSTANCE = new
DigitCharSequenceNormalizer();
+ // The ten ASCII digit strings, precomputed so the mapper does not allocate
a new single-char
+ // String for every digit it folds.
+ private static final String[] ASCII_DIGITS =
Review Comment:
Can this constant be defined somewhere in opennlp-api component? It has
generic character and offers potential for re-use in other components /
classes. Therefore, it should be a public constant.
##########
opennlp-core/opennlp-runtime/src/main/java/opennlp/tools/util/normalizer/Confusables.java:
##########
@@ -97,17 +105,14 @@ static Map<Integer, String> parse(InputStream in) throws
IOException {
final int firstSemicolon = content.indexOf(';');
final int secondSemicolon = content.indexOf(';', firstSemicolon + 1);
if (firstSemicolon < 0 || secondSemicolon < 0) {
- // A present-but-structurally-wrong line (fewer than two ';') is a
hard error, like the
Review Comment:
Please keep the previous code comment.
##########
opennlp-api/src/main/java/opennlp/tools/util/normalizer/Alignment.java:
##########
@@ -207,12 +219,38 @@ private static void checkRange(int start, int end, int
length) {
public static final class Builder {
private static final int MAX_ARRAY_SIZE = Integer.MAX_VALUE - 8;
+ private static final int DEFAULT_CAPACITY = 16;
- private int[] starts = new int[16];
- private int[] ends = new int[16];
+ private int[] starts;
+ private int[] ends;
private int count;
private int originalCursor;
+ /** Creates a builder with a small default capacity. */
Review Comment:
Please link "default capacity" with the constant for it in this 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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
