On Thu, 24 Oct 2024 14:57:19 GMT, Jonathan Lampérth <d...@openjdk.org> wrote:
> This PR includes changes to ensure `Code:` block indentation in `javap`for
> the `-verbose` case and non `-verbose` case is the same, which currently does
> not hold.
>
> Current behaviour of `javap` differs with and without `-verbose` in the
> following way:
> **Command**: `javap -c -XDdetails:stackMaps A.class`
>
> Without `-verbose`
>
>
> ...
> public void a();
> Code:
> 0: iconst_0
> 1: istore_1
> StackMap locals: this int
> StackMap stack:
> ...
>
>
> With `-verbose`
>
>
> ...
> public void a();
> descriptor: ()V
> flags: (0x0001) ACC_PUBLIC
> Code:
> stack=2, locals=2, args_size=1
> 0: iconst_0
> 1: istore_1
> StackMap locals: this int
> StackMap stack:
> ...
>
>
> With `-verbose` all contents of the `Code:` section include an extra (2
> space) indent, which is missing in the non `-verbose` case. This is because
> the `CodeWriter` is called via `CoderWriter.write(...)` in the `-verbose`
> case, which wraps the `Code:` block in `indent(+1);...indent(-1)`.
>
> In the non-verbose case this call is circumvented and a simplified version of
> `CoderWriter.write(...)` is included directly in `ClassWriter.writeMethod`.
>
> ---
>
> Alternatively to keep the logic within `CodeWriter` with the goal of keeping
> the logic for `-verbose` and non `-verbose` in the same place one could add
> `CodeWriter.writeSimple(...)`.
>
>
> void writeSimple(CodeAttribute attr) {
> println("Code:");
> indent(+1);
> writeInstrs(attr);
> writeExceptionTable(attr);
> indent(-1);
> }
>
>
> ---
>
> Note: Test setup is inspired by existing tests:
> [T6622232.java](https://github.com/openjdk/jdk/blob/master/test/langtools/tools/javap/T6622232.java)
> and
> [8244573](https://github.com/openjdk/jdk/blob/master/test/langtools/tools/javap/8244573)
I think I prefer your idea of keeping the code inside `CodeWriter`.
Maybe have a `write` and a `writeVerbose`, and have a private method with a
`boolean` param that controls printing of extra info?
test/langtools/tools/javap/8034066/T8034066.java line 37:
> 35: import java.util.regex.Pattern;
> 36:
> 37: public class T8034066 {
Please don't continue the old practice of naming tests after their bug ID. It
obfuscates which test class does what (both in the source code and in the test
logs). Suggestion for name: `TestStackMapDetailsIndent`
test/langtools/tools/javap/8034066/T8034066.java line 44:
> 42: public void run() throws IOException {
> 43: String output = javap();
> 44:
Could you put a comment here that roughly shows what the expected output looks
like? I think it would help readers understand what the following three lines
are looking for.
-------------
PR Review: https://git.openjdk.org/jdk/pull/21685#pullrequestreview-2393197370
PR Review Comment: https://git.openjdk.org/jdk/pull/21685#discussion_r1815355517
PR Review Comment: https://git.openjdk.org/jdk/pull/21685#discussion_r1815380775
