[PATCH] 8035271: Incorrect code attribute indentation

classic Classic list List threaded Threaded
4 messages Options
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

[PATCH] 8035271: Incorrect code attribute indentation

B. Blaser
Hi,

Next is a small fix to preserve the same code attribute indentation
for all javap options (issue 8035271), along with a test.

Bernard

diff --git a/src/jdk.jdeps/share/classes/com/sun/tools/javap/ClassWriter.java
b/src/jdk.jdeps/share/classes/com/sun/tools/javap/ClassWriter.java
--- a/src/jdk.jdeps/share/classes/com/sun/tools/javap/ClassWriter.java
+++ b/src/jdk.jdeps/share/classes/com/sun/tools/javap/ClassWriter.java
@@ -571,13 +571,17 @@
         } else if (code != null) {
             if (options.showDisassembled) {
                 println("Code:");
+                indent(+1); // Same as CodeWriter.write()
                 codeWriter.writeInstrs(code);
                 codeWriter.writeExceptionTable(code);
+                indent(-1);
             }

             if (options.showLineAndLocalVariableTables) {
+                indent(+1); // Same as CodeWriter.write()
                 attrWriter.write(code,
code.attributes.get(Attribute.LineNumberTable), constant_pool);
                 attrWriter.write(code,
code.attributes.get(Attribute.LocalVariableTable), constant_pool);
+                indent(-1);
             }
         }

diff --git a/test/tools/javap/CodeAttributeIndentation.java
b/test/tools/javap/CodeAttributeIndentation.java
new file mode 100644
--- /dev/null
+++ b/test/tools/javap/CodeAttributeIndentation.java
@@ -0,0 +1,63 @@
+/*
+ * @test
+ * @bug 8035271
+ * @summary Same code attribute indentation for all options.
+ * @library /tools/lib
+ * @modules jdk.compiler/com.sun.tools.javac.api
+ *          jdk.compiler/com.sun.tools.javac.main
+ *          jdk.jdeps/com.sun.tools.javap
+ * @build toolbox.ToolBox toolbox.JavacTask toolbox.JavapTask
+ * @run main CodeAttributeIndentation
+ */
+
+import toolbox.ToolBox;
+import toolbox.JavacTask;
+import toolbox.JavapTask;
+import toolbox.Task.OutputKind;
+
+public class CodeAttributeIndentation {
+    public static void main(String... args) {
+        String LS = System.getProperty("line.separator");
+
+        String src =
+            "public class A {" + LS +
+            "    public void a() {}" + LS +
+            "}";
+
+        String expected =
+            "Compiled from \"A.java\"" + LS +
+            "public class A {" + LS +
+            "  public A();" + LS +
+            "    Code:" + LS +
+            "         0: aload_0" + LS +
+            "         1: invokespecial #1                  " +
+            "// Method java/lang/Object.\"<init>\":()V" + LS +
+            "         4: return" + LS +
+            "      LineNumberTable:" + LS +
+            "        line 1: 0" + LS +
+            "      LocalVariableTable:" + LS +
+            "        Start  Length  Slot  Name   Signature" + LS +
+            "            0       5     0  this   LA;" + LS +
+            LS +
+            "  public void a();" + LS +
+            "    Code:" + LS +
+            "         0: return" + LS +
+            "      LineNumberTable:" + LS +
+            "        line 2: 0" + LS +
+            "      LocalVariableTable:" + LS +
+            "        Start  Length  Slot  Name   Signature" + LS +
+            "            0       1     0  this   LA;" + LS +
+            "}" + LS;
+
+        ToolBox tb = new ToolBox();
+
+        new JavacTask(tb).options("-g").sources(src).run();
+
+        String actual = new JavapTask(tb).options("-c",
"-l").classes("A.class")
+                .run().getOutput(OutputKind.DIRECT);
+
+        if (!actual.equals(expected))
+            throw new AssertionError("Incorrect code attribute indentation: " +
+                    LS + actual);
+    }
+}
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [PATCH] 8035271: Incorrect code attribute indentation

Jonathan Gibbons
Bernard,

In other tests with significant amounts of golden text, we have found it
more
readable to use \n in the golden strings, and use a single .replaceAll
on the string to change the \n to the platform newline character. This
avoids having to use + LS + at the end of each line.

I will look at your changeset in full context, but comments like
"// Same as CodeWriter.write()" may be meaningful now, but may be less
relevant a year or two down the road, prompting questions like "why does
this need to be stated?".

Separately, I note that the window of opportunity for fixes like this in
JDK 9
is rapidly closing, and at some point, we will have to push such fixes
to the
JDK 10 repos that were recently opened.

-- Jon

On 02/02/2017 09:05 AM, B. Blaser wrote:

> Hi,
>
> Next is a small fix to preserve the same code attribute indentation
> for all javap options (issue 8035271), along with a test.
>
> Bernard
>
> diff --git a/src/jdk.jdeps/share/classes/com/sun/tools/javap/ClassWriter.java
> b/src/jdk.jdeps/share/classes/com/sun/tools/javap/ClassWriter.java
> --- a/src/jdk.jdeps/share/classes/com/sun/tools/javap/ClassWriter.java
> +++ b/src/jdk.jdeps/share/classes/com/sun/tools/javap/ClassWriter.java
> @@ -571,13 +571,17 @@
>           } else if (code != null) {
>               if (options.showDisassembled) {
>                   println("Code:");
> +                indent(+1); // Same as CodeWriter.write()
>                   codeWriter.writeInstrs(code);
>                   codeWriter.writeExceptionTable(code);
> +                indent(-1);
>               }
>
>               if (options.showLineAndLocalVariableTables) {
> +                indent(+1); // Same as CodeWriter.write()
>                   attrWriter.write(code,
> code.attributes.get(Attribute.LineNumberTable), constant_pool);
>                   attrWriter.write(code,
> code.attributes.get(Attribute.LocalVariableTable), constant_pool);
> +                indent(-1);
>               }
>           }
>
> diff --git a/test/tools/javap/CodeAttributeIndentation.java
> b/test/tools/javap/CodeAttributeIndentation.java
> new file mode 100644
> --- /dev/null
> +++ b/test/tools/javap/CodeAttributeIndentation.java
> @@ -0,0 +1,63 @@
> +/*
> + * @test
> + * @bug 8035271
> + * @summary Same code attribute indentation for all options.
> + * @library /tools/lib
> + * @modules jdk.compiler/com.sun.tools.javac.api
> + *          jdk.compiler/com.sun.tools.javac.main
> + *          jdk.jdeps/com.sun.tools.javap
> + * @build toolbox.ToolBox toolbox.JavacTask toolbox.JavapTask
> + * @run main CodeAttributeIndentation
> + */
> +
> +import toolbox.ToolBox;
> +import toolbox.JavacTask;
> +import toolbox.JavapTask;
> +import toolbox.Task.OutputKind;
> +
> +public class CodeAttributeIndentation {
> +    public static void main(String... args) {
> +        String LS = System.getProperty("line.separator");
> +
> +        String src =
> +            "public class A {" + LS +
> +            "    public void a() {}" + LS +
> +            "}";
> +
> +        String expected =
> +            "Compiled from \"A.java\"" + LS +
> +            "public class A {" + LS +
> +            "  public A();" + LS +
> +            "    Code:" + LS +
> +            "         0: aload_0" + LS +
> +            "         1: invokespecial #1                  " +
> +            "// Method java/lang/Object.\"<init>\":()V" + LS +
> +            "         4: return" + LS +
> +            "      LineNumberTable:" + LS +
> +            "        line 1: 0" + LS +
> +            "      LocalVariableTable:" + LS +
> +            "        Start  Length  Slot  Name   Signature" + LS +
> +            "            0       5     0  this   LA;" + LS +
> +            LS +
> +            "  public void a();" + LS +
> +            "    Code:" + LS +
> +            "         0: return" + LS +
> +            "      LineNumberTable:" + LS +
> +            "        line 2: 0" + LS +
> +            "      LocalVariableTable:" + LS +
> +            "        Start  Length  Slot  Name   Signature" + LS +
> +            "            0       1     0  this   LA;" + LS +
> +            "}" + LS;
> +
> +        ToolBox tb = new ToolBox();
> +
> +        new JavacTask(tb).options("-g").sources(src).run();
> +
> +        String actual = new JavapTask(tb).options("-c",
> "-l").classes("A.class")
> +                .run().getOutput(OutputKind.DIRECT);
> +
> +        if (!actual.equals(expected))
> +            throw new AssertionError("Incorrect code attribute indentation: " +
> +                    LS + actual);
> +    }
> +}

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [PATCH] 8035271: Incorrect code attribute indentation

B. Blaser
Jonathan,

2017-02-02 18:41 GMT+01:00 Jonathan Gibbons <[hidden email]>:

> Bernard,
>
> In other tests with significant amounts of golden text, we have found it
> more
> readable to use \n in the golden strings, and use a single .replaceAll
> on the string to change the \n to the platform newline character. This
> avoids having to use + LS + at the end of each line.
>
> I will look at your changeset in full context, but comments like
> "// Same as CodeWriter.write()" may be meaningful now, but may be less
> relevant a year or two down the road, prompting questions like "why does
> this need to be stated?".
>
> Separately, I note that the window of opportunity for fixes like this in JDK
> 9
> is rapidly closing, and at some point, we will have to push such fixes to
> the
> JDK 10 repos that were recently opened.
>
> -- Jon

Thanks for your answer, I agree with your comments.
Please, find next the updated patch.

Bernard

> On 02/02/2017 09:05 AM, B. Blaser wrote:
>>
>> Hi,
>>
>> Next is a small fix to preserve the same code attribute indentation
>> for all javap options (issue 8035271), along with a test.
>>
>> Bernard

diff --git a/src/jdk.jdeps/share/classes/com/sun/tools/javap/ClassWriter.java
b/src/jdk.jdeps/share/classes/com/sun/tools/javap/ClassWriter.java
--- a/src/jdk.jdeps/share/classes/com/sun/tools/javap/ClassWriter.java
+++ b/src/jdk.jdeps/share/classes/com/sun/tools/javap/ClassWriter.java
@@ -571,13 +571,17 @@
         } else if (code != null) {
             if (options.showDisassembled) {
                 println("Code:");
+                indent(+1);
                 codeWriter.writeInstrs(code);
                 codeWriter.writeExceptionTable(code);
+                indent(-1);
             }

             if (options.showLineAndLocalVariableTables) {
+                indent(+1);
                 attrWriter.write(code,
code.attributes.get(Attribute.LineNumberTable), constant_pool);
                 attrWriter.write(code,
code.attributes.get(Attribute.LocalVariableTable), constant_pool);
+                indent(-1);
             }
         }

diff --git a/test/tools/javap/CodeAttributeIndentation.java
b/test/tools/javap/CodeAttributeIndentation.java
new file mode 100644
--- /dev/null
+++ b/test/tools/javap/CodeAttributeIndentation.java
@@ -0,0 +1,65 @@
+/*
+ * @test
+ * @bug 8035271
+ * @summary Same code attribute indentation for all options.
+ * @library /tools/lib
+ * @modules jdk.compiler/com.sun.tools.javac.api
+ *          jdk.compiler/com.sun.tools.javac.main
+ *          jdk.jdeps/com.sun.tools.javap
+ * @build toolbox.ToolBox toolbox.JavacTask toolbox.JavapTask
+ * @run main CodeAttributeIndentation
+ */
+
+import toolbox.ToolBox;
+import toolbox.JavacTask;
+import toolbox.JavapTask;
+import toolbox.Task.OutputKind;
+
+public class CodeAttributeIndentation {
+    public static void main(String... args) {
+        String LS = System.getProperty("line.separator");
+
+        String src = (
+            "public class A {\n" +
+            "    public void a() {}\n" +
+            "}"
+            ).replaceAll("\n", LS);
+
+        String expected = (
+            "Compiled from \"A.java\"\n" +
+            "public class A {\n" +
+            "  public A();\n" +
+            "    Code:\n" +
+            "         0: aload_0\n" +
+            "         1: invokespecial #1                  " +
+            "// Method java/lang/Object.\"<init>\":()V\n" +
+            "         4: return\n" +
+            "      LineNumberTable:\n" +
+            "        line 1: 0\n" +
+            "      LocalVariableTable:\n" +
+            "        Start  Length  Slot  Name   Signature\n" +
+            "            0       5     0  this   LA;\n" +
+            "\n" +
+            "  public void a();\n" +
+            "    Code:\n" +
+            "         0: return\n" +
+            "      LineNumberTable:\n" +
+            "        line 2: 0\n" +
+            "      LocalVariableTable:\n" +
+            "        Start  Length  Slot  Name   Signature\n" +
+            "            0       1     0  this   LA;\n" +
+            "}\n"
+            ).replaceAll("\n", LS);
+
+        ToolBox tb = new ToolBox();
+
+        new JavacTask(tb).options("-g").sources(src).run();
+
+        String actual = new JavapTask(tb).options("-c",
"-l").classes("A.class")
+                .run().getOutput(OutputKind.DIRECT);
+
+        if (!actual.equals(expected))
+            throw new AssertionError("Incorrect code attribute indentation: " +
+                    LS + actual);
+    }
+}
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [PATCH] 8035271: Incorrect code attribute indentation

B. Blaser
Hi,
Does this patch need to be improved?

Thanks,
Bernard

2017-02-06 15:02 GMT+01:00 B. Blaser <[hidden email]>:

> Jonathan,
>
> 2017-02-02 18:41 GMT+01:00 Jonathan Gibbons <[hidden email]>:
>> Bernard,
>>
>> In other tests with significant amounts of golden text, we have found it
>> more
>> readable to use \n in the golden strings, and use a single .replaceAll
>> on the string to change the \n to the platform newline character. This
>> avoids having to use + LS + at the end of each line.
>>
>> I will look at your changeset in full context, but comments like
>> "// Same as CodeWriter.write()" may be meaningful now, but may be less
>> relevant a year or two down the road, prompting questions like "why does
>> this need to be stated?".
>>
>> Separately, I note that the window of opportunity for fixes like this in JDK
>> 9
>> is rapidly closing, and at some point, we will have to push such fixes to
>> the
>> JDK 10 repos that were recently opened.
>>
>> -- Jon
>
> Thanks for your answer, I agree with your comments.
> Please, find next the updated patch.
>
> Bernard
>
>> On 02/02/2017 09:05 AM, B. Blaser wrote:
>>>
>>> Hi,
>>>
>>> Next is a small fix to preserve the same code attribute indentation
>>> for all javap options (issue 8035271), along with a test.
>>>
>>> Bernard
>
> diff --git a/src/jdk.jdeps/share/classes/com/sun/tools/javap/ClassWriter.java
> b/src/jdk.jdeps/share/classes/com/sun/tools/javap/ClassWriter.java
> --- a/src/jdk.jdeps/share/classes/com/sun/tools/javap/ClassWriter.java
> +++ b/src/jdk.jdeps/share/classes/com/sun/tools/javap/ClassWriter.java
> @@ -571,13 +571,17 @@
>          } else if (code != null) {
>              if (options.showDisassembled) {
>                  println("Code:");
> +                indent(+1);
>                  codeWriter.writeInstrs(code);
>                  codeWriter.writeExceptionTable(code);
> +                indent(-1);
>              }
>
>              if (options.showLineAndLocalVariableTables) {
> +                indent(+1);
>                  attrWriter.write(code,
> code.attributes.get(Attribute.LineNumberTable), constant_pool);
>                  attrWriter.write(code,
> code.attributes.get(Attribute.LocalVariableTable), constant_pool);
> +                indent(-1);
>              }
>          }
>
> diff --git a/test/tools/javap/CodeAttributeIndentation.java
> b/test/tools/javap/CodeAttributeIndentation.java
> new file mode 100644
> --- /dev/null
> +++ b/test/tools/javap/CodeAttributeIndentation.java
> @@ -0,0 +1,65 @@
> +/*
> + * @test
> + * @bug 8035271
> + * @summary Same code attribute indentation for all options.
> + * @library /tools/lib
> + * @modules jdk.compiler/com.sun.tools.javac.api
> + *          jdk.compiler/com.sun.tools.javac.main
> + *          jdk.jdeps/com.sun.tools.javap
> + * @build toolbox.ToolBox toolbox.JavacTask toolbox.JavapTask
> + * @run main CodeAttributeIndentation
> + */
> +
> +import toolbox.ToolBox;
> +import toolbox.JavacTask;
> +import toolbox.JavapTask;
> +import toolbox.Task.OutputKind;
> +
> +public class CodeAttributeIndentation {
> +    public static void main(String... args) {
> +        String LS = System.getProperty("line.separator");
> +
> +        String src = (
> +            "public class A {\n" +
> +            "    public void a() {}\n" +
> +            "}"
> +            ).replaceAll("\n", LS);
> +
> +        String expected = (
> +            "Compiled from \"A.java\"\n" +
> +            "public class A {\n" +
> +            "  public A();\n" +
> +            "    Code:\n" +
> +            "         0: aload_0\n" +
> +            "         1: invokespecial #1                  " +
> +            "// Method java/lang/Object.\"<init>\":()V\n" +
> +            "         4: return\n" +
> +            "      LineNumberTable:\n" +
> +            "        line 1: 0\n" +
> +            "      LocalVariableTable:\n" +
> +            "        Start  Length  Slot  Name   Signature\n" +
> +            "            0       5     0  this   LA;\n" +
> +            "\n" +
> +            "  public void a();\n" +
> +            "    Code:\n" +
> +            "         0: return\n" +
> +            "      LineNumberTable:\n" +
> +            "        line 2: 0\n" +
> +            "      LocalVariableTable:\n" +
> +            "        Start  Length  Slot  Name   Signature\n" +
> +            "            0       1     0  this   LA;\n" +
> +            "}\n"
> +            ).replaceAll("\n", LS);
> +
> +        ToolBox tb = new ToolBox();
> +
> +        new JavacTask(tb).options("-g").sources(src).run();
> +
> +        String actual = new JavapTask(tb).options("-c",
> "-l").classes("A.class")
> +                .run().getOutput(OutputKind.DIRECT);
> +
> +        if (!actual.equals(expected))
> +            throw new AssertionError("Incorrect code attribute indentation: " +
> +                    LS + actual);
> +    }
> +}
Loading...