You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@lucene.apache.org by GitBox <gi...@apache.org> on 2021/11/18 00:45:02 UTC
[GitHub] [lucene] rmuir opened a new pull request #452: LUCENE-10239: upgrade jflex (1.7.0 -> 1.8.2)
rmuir opened a new pull request #452:
URL: https://github.com/apache/lucene/pull/452
Upgrade jflex.
Change doesn't alter the behavior of any of the analyzers (unicode version or grammar refactorings), just the minimal to get new tooling working.
--
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: issues-unsubscribe@lucene.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
[GitHub] [lucene] rmuir commented on pull request #452: LUCENE-10239: upgrade jflex (1.7.0 -> 1.8.2)
Posted by GitBox <gi...@apache.org>.
rmuir commented on pull request #452:
URL: https://github.com/apache/lucene/pull/452#issuecomment-974001822
@dweiss see https://issues.apache.org/jira/browse/LUCENE-5897 for more background on that
--
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: issues-unsubscribe@lucene.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
[GitHub] [lucene] rmuir commented on a change in pull request #452: LUCENE-10239: upgrade jflex (1.7.0 -> 1.8.2)
Posted by GitBox <gi...@apache.org>.
rmuir commented on a change in pull request #452:
URL: https://github.com/apache/lucene/pull/452#discussion_r751799701
##########
File path: dev-tools/missing-doclet/src/main/java/org/apache/lucene/missingdoclet/MissingDoclet.java
##########
@@ -331,7 +331,6 @@ private boolean hasInheritedJavadocs(Element element) {
if (hasOverrides) {
// If an element has explicit @Overrides annotation, assume it does
// have inherited javadocs somewhere.
- reporter.print(Diagnostic.Kind.NOTE, element, "javadoc empty but @Override declared, skipping.");
Review comment:
If javadocs are missing, the noise from these INFO/NOTEs (there is a ton of them) makes it really difficult to see what is truly failing the build. It floods the output.
--
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: issues-unsubscribe@lucene.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
[GitHub] [lucene] dweiss commented on pull request #452: LUCENE-10239: upgrade jflex (1.7.0 -> 1.8.2)
Posted by GitBox <gi...@apache.org>.
dweiss commented on pull request #452:
URL: https://github.com/apache/lucene/pull/452#issuecomment-973995491
I tried to look up why this no-buffer-expansion is needed. I see LUCENE-8527 and some corner cases there... but why is it used here and there and not all across the board (some tokenizers use the default and others use the no-buffer version).
--
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: issues-unsubscribe@lucene.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
[GitHub] [lucene] rmuir commented on pull request #452: LUCENE-10239: upgrade jflex (1.7.0 -> 1.8.2)
Posted by GitBox <gi...@apache.org>.
rmuir commented on pull request #452:
URL: https://github.com/apache/lucene/pull/452#issuecomment-973323310
For convenience of reviewing: here is the diff between default skeleton and "buffer-expansion-disabled" skeleton. It is kinda the only way to review it since we brought in all the upstream changes.
```
think:lucene[LUCENE-10239]$ diff -u gradle/generation/jflex/skeleton.default.txt gradle/generation/jflex/skeleton.disable.buffer.expansion.txt
--- gradle/generation/jflex/skeleton.default.txt 2021-11-17 19:04:46.844620167 -0500
+++ gradle/generation/jflex/skeleton.disable.buffer.expansion.txt 2021-11-17 19:05:01.124853267 -0500
@@ -5,7 +5,7 @@
/** Initial size of the lookahead buffer. */
--- private static final int ZZ_BUFFERSIZE = ...;
- /** Lexical states. */
+ /** Lexical States. */
--- lexical states, charmap
/** Error code for "Unknown internal scanner error". */
@@ -94,18 +94,11 @@
zzStartRead = 0;
}
- /* is the buffer big enough? */
- if (zzCurrentPos >= zzBuffer.length - zzFinalHighSurrogate) {
- /* if not: blow it up */
- char newBuffer[] = new char[zzBuffer.length * 2];
- System.arraycopy(zzBuffer, 0, newBuffer, 0, zzBuffer.length);
- zzBuffer = newBuffer;
- zzEndRead += zzFinalHighSurrogate;
- zzFinalHighSurrogate = 0;
- }
-
/* fill the buffer with new input */
- int requested = zzBuffer.length - zzEndRead;
+ int requested = zzBuffer.length - zzEndRead - zzFinalHighSurrogate;
+ if (requested == 0) {
+ return true;
+ }
int numRead = zzReader.read(zzBuffer, zzEndRead, requested);
/* not supposed to occur according to specification of java.io.Reader */
@@ -119,6 +112,9 @@
if (numRead == requested) { // We requested too few chars to encode a full Unicode character
--zzEndRead;
zzFinalHighSurrogate = 1;
+ if (numRead == 1) {
+ return true;
+ }
} else { // There is room in the buffer for at least one more char
int c = zzReader.read(); // Expecting to read a paired low surrogate char
if (c == -1) {
```
--
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: issues-unsubscribe@lucene.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
[GitHub] [lucene] rmuir commented on a change in pull request #452: LUCENE-10239: upgrade jflex (1.7.0 -> 1.8.2)
Posted by GitBox <gi...@apache.org>.
rmuir commented on a change in pull request #452:
URL: https://github.com/apache/lucene/pull/452#discussion_r752297406
##########
File path: dev-tools/missing-doclet/src/main/java/org/apache/lucene/missingdoclet/MissingDoclet.java
##########
@@ -331,7 +331,6 @@ private boolean hasInheritedJavadocs(Element element) {
if (hasOverrides) {
// If an element has explicit @Overrides annotation, assume it does
// have inherited javadocs somewhere.
- reporter.print(Diagnostic.Kind.NOTE, element, "javadoc empty but @Override declared, skipping.");
Review comment:
yeah, i just havent experimented with all the options: such as default level vs `-verbose` vs `-quiet`
--
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: issues-unsubscribe@lucene.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
[GitHub] [lucene] dweiss commented on a change in pull request #452: LUCENE-10239: upgrade jflex (1.7.0 -> 1.8.2)
Posted by GitBox <gi...@apache.org>.
dweiss commented on a change in pull request #452:
URL: https://github.com/apache/lucene/pull/452#discussion_r751944910
##########
File path: dev-tools/missing-doclet/src/main/java/org/apache/lucene/missingdoclet/MissingDoclet.java
##########
@@ -331,7 +331,6 @@ private boolean hasInheritedJavadocs(Element element) {
if (hasOverrides) {
// If an element has explicit @Overrides annotation, assume it does
// have inherited javadocs somewhere.
- reporter.print(Diagnostic.Kind.NOTE, element, "javadoc empty but @Override declared, skipping.");
Review comment:
Maybe it could be a parameter of the doclet to emit these conditionally? Does it make any sense?
--
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: issues-unsubscribe@lucene.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
[GitHub] [lucene] rmuir commented on pull request #452: LUCENE-10239: upgrade jflex (1.7.0 -> 1.8.2)
Posted by GitBox <gi...@apache.org>.
rmuir commented on pull request #452:
URL: https://github.com/apache/lucene/pull/452#issuecomment-974115268
thank you @dweiss and @sarowe for reviewing.
--
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: issues-unsubscribe@lucene.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
[GitHub] [lucene] rmuir commented on pull request #452: LUCENE-10239: upgrade jflex (1.7.0 -> 1.8.2)
Posted by GitBox <gi...@apache.org>.
rmuir commented on pull request #452:
URL: https://github.com/apache/lucene/pull/452#issuecomment-972384034
I thought to try to do a large refactoring here and quickly backed off, I think let's just upgrade to the latest jflex as a standalone change.
The trickiest parts (and ones needing close review):
1. the skeleton files. I reviewed `diff -u` of the two current skeleton files, making sure i understood the changes needed to work without buffer expansion. Then I updated the default skeleton file from jflex sources (there are many changes) and created the "without buffer expansion" version with appropriate logic.
2. jflex change to support > 2GB files. Some variables become `long` and we have to change some code to work with it. In general Lucene won't work with such files (e.g. `OffsetAttribute` is based on `int`), so I tried to just add minimal casts. I'd rather not use `Math.toIntExact` as it may impact performance. If we want to improve safety, maybe it should be via a cheaper mechanism.
3. warnings suppression. jflex starts to try to suppress its own warnings, but they do the warning in a nonstandard way, and you can't stack these annotations, so we have to add a little hack. I commented on the issues in the jflex bug tracker (linked in the gradle hack).
--
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: issues-unsubscribe@lucene.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
[GitHub] [lucene] rmuir commented on a change in pull request #452: LUCENE-10239: upgrade jflex (1.7.0 -> 1.8.2)
Posted by GitBox <gi...@apache.org>.
rmuir commented on a change in pull request #452:
URL: https://github.com/apache/lucene/pull/452#discussion_r752274558
##########
File path: dev-tools/missing-doclet/src/main/java/org/apache/lucene/missingdoclet/MissingDoclet.java
##########
@@ -331,7 +331,6 @@ private boolean hasInheritedJavadocs(Element element) {
if (hasOverrides) {
// If an element has explicit @Overrides annotation, assume it does
// have inherited javadocs somewhere.
- reporter.print(Diagnostic.Kind.NOTE, element, "javadoc empty but @Override declared, skipping.");
Review comment:
We could add back the debugging guarded by a sysprop. maybe we could enable additional output from the gradle build if someone runs `gradlew --debug`? I keep thinking there should already be a "proper" way for this (e.g. cmdline flag to javadocs tool), but I'm not finding it.
--
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: issues-unsubscribe@lucene.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
[GitHub] [lucene] rmuir commented on a change in pull request #452: LUCENE-10239: upgrade jflex (1.7.0 -> 1.8.2)
Posted by GitBox <gi...@apache.org>.
rmuir commented on a change in pull request #452:
URL: https://github.com/apache/lucene/pull/452#discussion_r751800229
##########
File path: gradle/generation/jflex.gradle
##########
@@ -270,6 +270,17 @@ class JFlexTask extends DefaultTask {
]
}
+ // fix invalid SuppressWarnings from jflex, so that it works with javac.
+ // we also need to suppress 'unused' because of dead code, so it passes ecjLint
+ // you can't "stack" SuppressWarnings annotations, jflex adds its own, this is the only way
+ // https://github.com/jflex-de/jflex/issues/762
+ project.ant.replace(
+ file: target,
+ encoding: "UTF-8",
+ token: 'SuppressWarnings("FallThrough")',
+ value: 'SuppressWarnings({"fallthrough","unused"})'
+ )
+
Review comment:
This is the hack we have to do for now, see https://github.com/jflex-de/jflex/issues/762 where a method is being discussed to customize the suppress warnings without find-replace
--
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: issues-unsubscribe@lucene.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
[GitHub] [lucene] dweiss commented on a change in pull request #452: LUCENE-10239: upgrade jflex (1.7.0 -> 1.8.2)
Posted by GitBox <gi...@apache.org>.
dweiss commented on a change in pull request #452:
URL: https://github.com/apache/lucene/pull/452#discussion_r752281787
##########
File path: dev-tools/missing-doclet/src/main/java/org/apache/lucene/missingdoclet/MissingDoclet.java
##########
@@ -331,7 +331,6 @@ private boolean hasInheritedJavadocs(Element element) {
if (hasOverrides) {
// If an element has explicit @Overrides annotation, assume it does
// have inherited javadocs somewhere.
- reporter.print(Diagnostic.Kind.NOTE, element, "javadoc empty but @Override declared, skipping.");
Review comment:
I think that code is sort of correct logging at NOTE level - the problem is I can't find the right option in javadoc to skip NOTE messages (and leave only warnings). I'd comment it out instead of removing it so that perhaps it can be tackled separately. There should be a way of doing it somehow.
--
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: issues-unsubscribe@lucene.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
[GitHub] [lucene] rmuir merged pull request #452: LUCENE-10239: upgrade jflex (1.7.0 -> 1.8.2)
Posted by GitBox <gi...@apache.org>.
rmuir merged pull request #452:
URL: https://github.com/apache/lucene/pull/452
--
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: issues-unsubscribe@lucene.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org