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.

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 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 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

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

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.

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