1508178 - Figure out the workaround for the stack transition comment in BytecodeEmitter and helper classes for clang-format

Status: RESOLVED FIXED

(Core :: JavaScript Engine, enhancement)

Description

[Tooru Fujisawa [:arai]]

BytecodeEmitter and helper classes are using single line comment for describing the stack transition,
which is written in the same line as emit* method call, or in the next line if the emit* line is long.

for example, some code from ForOfEmitter.cpp:

    if (!bce_->emit1(JSOP_POP)) {                     // NEXT ITER
        return false;
    }
    if (!bce_->emit1(JSOP_DUP2)) {                    // NEXT ITER NEXT ITER
        return false;
    }

    if (!bce_->emitIteratorNext(forPos, iterKind_, allowSelfHostedIter_)) {
        return false;                                 // NEXT ITER RESULT
    }

which is modified like the following after clang-format.

    if (!bce_->emit1(JSOP_POP)) { // NEXT ITER
        return false;
    }
    if (!bce_->emit1(JSOP_DUP2)) { // NEXT ITER NEXT ITER
        return false;
    }

    if (!bce_->emitIteratorNext(forPos, iterKind_, allowSelfHostedIter_)) {
        return false; // NEXT ITER RESULT
    }

we need to figure out some workaround to keep those comments aligned
maybe put them in their own lines or so.

Comment 1

[Tooru Fujisawa [:arai]]

given the tab-width is changed from 4 spaces to 2 spaces, alignment will break between multiple comments across different block level,
and I think applying change twice (+ clang-format) is simpler.
  1. move the comment to its own line to minimize the effect of clang-format
  2. apply clang-format
  3. align the stack comment to the same level

to achieve that, I'm going to create a script that fixes the comment alignment, in the step 3.

also, moving the stack transition comment to its own line benefits that we can have more spaces.
maybe we could start from column <40.

Comment 2

[Tooru Fujisawa [:arai]]

here's the result after step 1:

    if (!bce_->emit1(JSOP_POP)) {
        //                    [stack] NEXT ITER
        return false;
    }
    if (!bce_->emit1(JSOP_DUP2)) {
        //                    [stack] NEXT ITER NEXT ITER
        return false;
    }

    if (!bce_->emitIteratorNext(forPos, iterKind_, allowSelfHostedIter_)) {
        //                    [stack] NEXT ITER RESULT
        return false;
    }

I put "[stack]" marker to the all stack transition comments, so that the automation in the step 3 can align them.

Comment 3

[Tooru Fujisawa [:arai]]

Attachment: Part 1: Move the stack transition comment in BytecodeEmitter and helper classes into its own line, and align globally.

Just comment-only fix.
Changed the stack transition comment to follow the comment #2's rule,
that is, put the "//   [stack] ..." comment in the next line of the emit,
which is basically the then-branch of "if (!emit...)".

the "[stack]" is aligned to 30 column across files, by the script in Part 2.
I'm about to apply the script again after clang-format, given it will change the indent-width and that will result in mis-alignment.

Comment 4

[Tooru Fujisawa [:arai]]

Attachment: Part 2: Add a script to align the stack transition comment.

The script mentioned in Part 1.

running "js/src/frontend/align_stack_comment.py js/src/frontend/BytecodeEmitter.cpp" aligns the stack transition comment in BytecodeEmitter.cpp,
so that "[" of "[stack]" starts from column 30.

Comment 5

[Jason Orendorff [:jorendorff]]

Comment on attachment 9025990 [details] [diff] [review]
Part 1: Move the stack transition comment in BytecodeEmitter and helper classes into its own line, and align globally.

Review of attachment 9025990 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/src/frontend/BytecodeEmitter.cpp
@@ +7395,5 @@
>              return false;
>          }
>          for (ParseNode* arg : argsList->contents()) {
>              if (!emitTree(arg)) {
> +                //            [stack] CALLEE THIS *ARG

Put the star after the identifier, please:  `ARG*`

I see we are already using a leading star in some destructuring code, but I think that is wrong. This notation should be like regular expressions. It's a Kleene star. `*SET` should be `SET?`, and `*LREF` should be either `LREF?` or `LREF*` (I didn't check).

Comment 6

[Jason Orendorff [:jorendorff]]

Comment on attachment 9025991 [details] [diff] [review]
Part 2: Add a script to align the stack transition comment.

Review of attachment 9025991 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/src/frontend/align_stack_comment.py
@@ +76,5 @@
> +
> +
> +if __name__ == '__main__':
> +    if len(sys.argv) < 2:
> +        print('Usage: align_stack_cmment.py FILE',

typo: cmment

Comment 7

[Tooru Fujisawa [:arai]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/936db6d8a4733822b544355138e82a96f49fd085
Bug 1508178 - Part 1: Move the stack transition comment in BytecodeEmitter and helper classes into its own line, and align globally. r=jorendorff

https://hg.mozilla.org/integration/mozilla-inbound/rev/91a0fe92cc4b1b8a09ce6671be194318be0bb25d
Bug 1508178 - Part 2: Add a script to align the stack transition comment. r=jorendorff

Comment 8

[Jan de Mooij [:jandem]]

arai, can we close this bug? I don't see the leave-open keyword so I assume the sheriffs forgot?

Comment 9

[Tooru Fujisawa [:arai]]

https://hg.mozilla.org/mozilla-central/rev/936db6d8a473
https://hg.mozilla.org/mozilla-central/rev/91a0fe92cc4b

Comment 10

[Ted Campbell [:tcampbell]]

The current code and ESR differ quite a bit due to all the Emitter work that arai has done this year as well as the { } reformat work. Uplifts near this code will likely be fairly manual, so I'd like to mark this comment fixup work as wontfix for ESR60.

Comment 11

[(no longer active)]

https://hg.mozilla.org/releases/mozilla-esr60/rev/6fe7b0d029fd
https://hg.mozilla.org/releases/mozilla-esr60/rev/4131da013f8f