8254073: Tokenizer improvements (revised) #525

JimLaskey · 2020-10-06T14:01:11Z

This is a full revision of #435 which contained two 'out by one' bugs and was reverted.

This revision contains the changes of that pull request plus:

diff --git a/src/jdk.compiler/share/classes/com/sun/tools/javac/parser/JavadocTokenizer.java b/src/jdk.compiler/share/classes/com/sun/tools/javac/parser/JavadocTokenizer.java
index 39d9eadcf3a..b8425ad1ecb 100644
--- a/src/jdk.compiler/share/classes/com/sun/tools/javac/parser/JavadocTokenizer.java
+++ b/src/jdk.compiler/share/classes/com/sun/tools/javac/parser/JavadocTokenizer.java
@@ -306,8 +306,9 @@ public class JavadocTokenizer extends JavaTokenizer {
      *
      * Thus, to find the source position of any position, p, in the comment
      * string, find the index, i, of the pair whose string offset
-     * ({@code map[i + SB_OFFSET] }) is closest to but not greater than p. Then,
-     * {@code sourcePos(p) = map[i + POS_OFFSET] + (p - map[i + SB_OFFSET]) }.
+     * ({@code map[i * NOFFSETS + SB_OFFSET] }) is closest to but not greater
+     * than p. Then, {@code sourcePos(p) = map[i * NOFFSETS + POS_OFFSET] +
+     *                                (p - map[i * NOFFSETS + SB_OFFSET]) }.
      */
     static class OffsetMap {
         /**
@@ -426,7 +427,7 @@ public class JavadocTokenizer extends JavaTokenizer {
             int start = 0;
             int end = size / NOFFSETS;
 
-            while (start < end - NOFFSETS) {
+            while (start < end - 1) {
                 // find an index midway between start and end
                 int index = (start + end) / 2;
                 int indexScaled = index * NOFFSETS;
diff --git a/src/jdk.compiler/share/classes/com/sun/tools/javac/parser/UnicodeReader.java b/src/jdk.compiler/share/classes/com/sun/tools/javac/parser/UnicodeReader.java
index 2472632dbcd..7584b79044b 100644
--- a/src/jdk.compiler/share/classes/com/sun/tools/javac/parser/UnicodeReader.java
+++ b/src/jdk.compiler/share/classes/com/sun/tools/javac/parser/UnicodeReader.java
@@ -221,48 +221,49 @@ public class UnicodeReader {
     private boolean unicodeEscape() {
         // Start of unicode escape (past backslash.)
         int start = position + width;
-        int index;
+
+        // Default to backslash result, unless proven otherwise.
+        character = '\\';
+        width = 1;
 
         // Skip multiple 'u'.
+        int index;
         for (index = start; index < length; index++) {
             if (buffer[index] != 'u') {
                 break;
             }
         }
 
-        // Needs to be at least backslash-u.
-        if (index != start) {
-            // If enough characters available.
-            if (index + 4 < length) {
-                // Convert four hex digits to codepoint. If any digit is invalid then the
-                // result is negative.
-                int code = (Character.digit(buffer[index++], 16) << 12) |
-                           (Character.digit(buffer[index++], 16) << 8) |
-                           (Character.digit(buffer[index++], 16) << 4) |
-                            Character.digit(buffer[index++], 16);
-
-                // If all digits are good.
-                if (code >= 0) {
-                    width = index - position;
-                    character = (char)code;
-
-                    return true;
-                }
-            }
+        // Needs to have been at least one u.
+        if (index == start) {
+            return false;
+        }
 
-            // Did not work out.
-            log.error(position, Errors.IllegalUnicodeEsc);
-            width = index - position;
+        int code = 0;
 
-            // Return true so that the invalid unicode escape is skipped.
-            return true;
+        for (int i = 0; i < 4; i++) {
+            int digit = Character.digit(buffer[index], 16);
+            code = code << 4 | digit;
+
+            if (code < 0) {
+                break;
+            }
+
+            index++;
         }
 
-        // Must be just a backslash.
-        character = '\\';
-        width = 1;
+        // Skip digits even if error.
+        width = index - position;
 
-        return false;
+        // If all digits are good.
+        if (code >= 0) {
+            character = (char)code;
+        } else {
+            log.error(position, Errors.IllegalUnicodeEsc);
+        }
+
+        // Return true even if error so that the invalid unicode escape is skipped.
+        return true;
     }
 
     /**
@@ -549,7 +550,7 @@ public class UnicodeReader {
         /**
          * Offset from the beginning of the original reader buffer.
          */
-        private int offset;
+        final private int offset;
 
         /**
          * Current column in the comment.

Progress

Change must not contain extraneous whitespace
Commit message must refer to an issue
Change must be properly reviewed

Issue

JDK-8254073: Tokenizer improvements (revised)

Reviewers

Maurizio Cimadamore (@mcimadamore - Reviewer) ⚠️ Review applies to db7a206

Download

$ git fetch https://git.openjdk.java.net/jdk pull/525/head:pull/525
$ git checkout pull/525

JimLaskey · 2020-10-06T14:01:41Z

/test

bridgekeeper · 2020-10-06T14:01:46Z

👋 Welcome back jlaskey! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

openjdk · 2020-10-06T14:02:52Z

@JimLaskey The following label will be automatically applied to this pull request:

compiler

When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing list. If you would like to change these labels, use the /label pull request command.

openjdk · 2020-10-06T14:04:05Z

Could not create test job

mlbridge · 2020-10-06T14:07:14Z

Webrevs

JimLaskey · 2020-10-06T14:14:44Z

/test

openjdk · 2020-10-06T14:15:13Z

Could not create test job

mcimadamore · 2020-10-06T14:37:32Z

src/jdk.compiler/share/classes/com/sun/tools/javac/parser/UnicodeReader.java

+        int code = 0;
+
+        for (int i = 0; i < 4; i++) {
+            int digit = Character.digit(buffer[index], 16);


This looks suspicious - what if index ends up being bigger than (or equal to) buffer.length ?
Maybe we need a test for incomplete unicode sequences at the end of the tokenizer input - e.g. \u123

You are correct. Will revise.

Added change to

int digit = index < length ? Character.digit(buffer[index], 16) : -1;

Also added new test.

mcimadamore

Looks good - I've added optional suggestions for the test

mcimadamore · 2020-10-06T20:48:34Z

test/langtools/tools/javac/lexer/JavaLexerTest2.java

+
+public class JavaLexerTest2 {
+    static final TestTuple[] TESTS = {
+            new TestTuple("0bL",         LONGLITERAL, true),


Stylistic (optional) comment - we could have a common TestTuple superclass and two subclasses called Success and Failure, so that, by looking at the TESTS array it will be apparent what the behavior should be

mcimadamore · 2020-10-06T20:48:49Z

test/langtools/tools/javac/lexer/JavaLexerTest2.java

+        String expected;
+        boolean willFail;
+
+        TestTuple(String input, TokenKind kind, String expected, boolean willFail) {


Is anything using this?

It does now.

mcimadamore · 2020-10-06T20:49:19Z

test/langtools/tools/javac/lexer/JavaLexerTest2.java

+
+import static com.sun.tools.javac.parser.Tokens.TokenKind.*;
+
+public class JavaLexerTest2 {


Should we merge JavaLexerTest and this one? After all you have all the required infra in here?

openjdk · 2020-10-06T20:50:22Z

@JimLaskey This change now passes all automated pre-integration checks.

ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details.

After integration, the commit message for the final commit will be:

8254073: Tokenizer improvements (revised)

Reviewed-by: mcimadamore

You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed.

At the time when this comment was updated there had been no new commits pushed to the master branch. If another commit should be pushed before you perform the /integrate command, your PR will be automatically rebased. If you prefer to avoid any potential automatic rebasing, please check the documentation for the /integrate command for further details.

➡️ To integrate this PR with the above commit message to the master branch, type /integrate in a new comment.

JimLaskey · 2020-10-07T11:53:26Z

/test

openjdk · 2020-10-07T11:53:44Z

Could not create test job

mcimadamore

Looks good

JimLaskey · 2020-10-09T11:07:44Z

/integrate

openjdk · 2020-10-09T11:09:14Z

@JimLaskey Since your change was applied there have been 10 commits pushed to the master branch:

9cecc16: 8254244: Some code emitted by TemplateTable::branch is unused when running TieredCompilation
a95590d: 8254285: G1: Remove "What is this about" comment in G1CollectedHeap.cpp
0230781: 8254175: Build no-pch configuration in debug mode for submit checks
b9873e1: 8253180: ZGC: Implementation of JEP 376: ZGC: Concurrent Thread-Stack Processing
a2f6519: 8233685: Test tools/javac/modules/AddLimitMods.java fails
70be8c7: 8253965: Delete the outdated java.awt.PeerFixer class
ced46b1: 8254190: [s390] interpreter misses exception check after calling monitorenter
5351ba6: 8254262: jdk.test.lib.Utils::createTemp* don't pass attrs
8c0d3d7: 8254195: java/nio/file/Files/SubstDrive.java failed with "AssertionError: expected [144951656448] but found [144951640064]"
c2a5de6: 8253681: closed java/awt/dnd/MouseEventAfterStartDragTest/MouseEventAfterStartDragTest.html test failed

Your commit was automatically rebased without conflicts.

Pushed as commit 4f9a1ff.

💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored.

JimLaskey added 3 commits October 6, 2020 09:05

8254073: Tokenizer improvements (revised)

6969aa7

Merge branch 'master' into 8254073

eab2461

Merge branch 'master' into 8254073

0879e7b

JimLaskey marked this pull request as ready for review October 6, 2020 14:01

openjdk bot added the rfr Pull request is ready for review label Oct 6, 2020

openjdk bot added the compiler compiler-dev@openjdk.org label Oct 6, 2020

mcimadamore reviewed Oct 6, 2020

View reviewed changes

8254073: Tokenizer improvements (revised)

6d6b3ee

mcimadamore approved these changes Oct 6, 2020

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label Oct 6, 2020

JimLaskey added 3 commits October 7, 2020 10:55

8254073: Tokenizer improvements (revised)

6b31c79

8254073: Tokenizer improvements (revised)

66625bd

Merge branch 'master' into 8254073

db7a206

mcimadamore approved these changes Oct 7, 2020

View reviewed changes

JimLaskey added 5 commits October 8, 2020 08:34

Merge branch 'master' into 8254073

05eb6c5

8254073: Tokenizer improvements (revised)

78ebcd0

Merge branch 'master' into 8254073

53ff536

8254073: Tokenizer improvements (revised)

8e090fc

Merge branch 'master' into 8254073

d12ee28

openjdk bot closed this Oct 9, 2020

openjdk bot added integrated Pull request has been integrated and removed ready Pull request is ready to be integrated rfr Pull request is ready for review labels Oct 9, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

8254073: Tokenizer improvements (revised) #525

8254073: Tokenizer improvements (revised) #525

JimLaskey commented Oct 6, 2020 •

edited by openjdk bot

Loading

JimLaskey commented Oct 6, 2020

bridgekeeper bot commented Oct 6, 2020

openjdk bot commented Oct 6, 2020

openjdk bot commented Oct 6, 2020

mlbridge bot commented Oct 6, 2020 •

edited

Loading

JimLaskey commented Oct 6, 2020

openjdk bot commented Oct 6, 2020

mcimadamore Oct 6, 2020

JimLaskey Oct 6, 2020

JimLaskey Oct 6, 2020

mcimadamore left a comment •

edited

Loading

mcimadamore Oct 6, 2020

JimLaskey Oct 7, 2020

mcimadamore Oct 6, 2020

JimLaskey Oct 7, 2020

mcimadamore Oct 6, 2020

JimLaskey Oct 7, 2020

openjdk bot commented Oct 6, 2020 •

edited

Loading

JimLaskey commented Oct 7, 2020

openjdk bot commented Oct 7, 2020

mcimadamore left a comment

JimLaskey commented Oct 9, 2020

openjdk bot commented Oct 9, 2020


		import static com.sun.tools.javac.parser.Tokens.TokenKind.*;

		public class JavaLexerTest2 {

8254073: Tokenizer improvements (revised) #525

8254073: Tokenizer improvements (revised) #525

Conversation

JimLaskey commented Oct 6, 2020 • edited by openjdk bot Loading

Progress

Issue

Reviewers

Download

JimLaskey commented Oct 6, 2020

bridgekeeper bot commented Oct 6, 2020

openjdk bot commented Oct 6, 2020

openjdk bot commented Oct 6, 2020

mlbridge bot commented Oct 6, 2020 • edited Loading

Webrevs

JimLaskey commented Oct 6, 2020

openjdk bot commented Oct 6, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mcimadamore left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

openjdk bot commented Oct 6, 2020 • edited Loading

JimLaskey commented Oct 7, 2020

openjdk bot commented Oct 7, 2020

mcimadamore left a comment

Choose a reason for hiding this comment

JimLaskey commented Oct 9, 2020

openjdk bot commented Oct 9, 2020

JimLaskey commented Oct 6, 2020 •

edited by openjdk bot

Loading

mlbridge bot commented Oct 6, 2020 •

edited

Loading

mcimadamore left a comment •

edited

Loading

openjdk bot commented Oct 6, 2020 •

edited

Loading