Closed Bug 1508178 Opened 9 months ago Closed 9 months ago

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

Categories

(Core :: JavaScript Engine, enhancement)

enhancement
Not set

Tracking

()

RESOLVED FIXED
mozilla65
Tracking Status
firefox-esr60 --- wontfix
firefox65 --- fixed

People

(Reporter: arai, Assigned: arai)

References

Details

Attachments

(2 files)

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.
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.
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.
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.
Attachment #9025990 - Flags: review?(jorendorff)
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.
Attachment #9025991 - Flags: review?(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).
Attachment #9025990 - Flags: review?(jorendorff) → review+
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
Attachment #9025991 - Flags: review?(jorendorff) → review+
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
arai, can we close this bug? I don't see the leave-open keyword so I assume the sheriffs forgot?
https://hg.mozilla.org/mozilla-central/rev/936db6d8a473
https://hg.mozilla.org/mozilla-central/rev/91a0fe92cc4b
Status: ASSIGNED → RESOLVED
Closed: 9 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla65
Blocks: 1511385
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.
You need to log in before you can comment on or make changes to this bug.