Tooru Fujisawa [:arai] Assignee
	Description • 6 years ago

Currently we're using magic number like 0, 1, 2 for setSrcNoteOffset arguments.
at least we should add constant or enum, to make it clear which offset it is.

	Tooru Fujisawa [:arai] Assignee
	Comment 1 • 6 years ago

Applied to source notes for switch/case
(also, fixed SwitchEmitter::emitCaseOrDefaultJump not to add unnecessary source note, which is found while preparing this patch)
Can I have some feedback?

The plan is following:
  * create classes for each source note type (TableSwitch/CondSwitch/NextCase here),
    and add enum for each offset (I used "Fieilds" as enum name since it can hold non-offset)
  * use the enum when calling BytecodeEmitter::setSrcNoteOffset or GetSrcNoteOffset
  * put everything in SrcNote class (or maybe namespace)
  * move other related code to those classes in followup bugs
    * maybe we could make them accessor, instead of public enum
    * add wrapper for GetSrcNote for each type, which asserts opcode and SN_TYPE
    * move SN_* macros/functions into SrcNote or inner classes

I'm about to fix each source note type separately, at the same time as adding BytecodeEmitter helper classes (bug 1456404 etc)

	Tooru Fujisawa [:arai] Assignee
	Comment 2 • 6 years ago

also applied to try.
(simpler than Part 1)

	Jan de Mooij [:jandem]
	Comment 3 • 6 years ago

Comment on attachment 8994116 [details] [diff] [review]
Part 1: Add source note field constants for switch.

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

Fantastic! Definitely an improvement.

::: js/src/frontend/SourceNotes.h
@@ +49,5 @@
>      M(SRC_CONTINUE,     "continue",    0)  /* JSOP_GOTO is a continue. */                          \
>      M(SRC_BREAK,        "break",       0)  /* JSOP_GOTO is a break. */                             \
>      M(SRC_BREAK2LABEL,  "break2label", 0)  /* JSOP_GOTO for 'break label'. */                      \
>      M(SRC_SWITCHBREAK,  "switchbreak", 0)  /* JSOP_GOTO is a break in a switch. */                 \
> +    M(SRC_TABLESWITCH,  "tableswitch", 1) \

Your plan is to derive this last column automatically, right? (Each enum could have "Count" (or something) as the last value.)

@@ +82,5 @@
> +  public:
> +    // SRC_TABLESWITCH: Source note for JSOP_TABLESWITCH.
> +    class TableSwitch {
> +      public:
> +        enum Fields {

This class + enum could be |enum class TableSwitch|, but I assume you'll add more fields to the classes :)

::: js/src/frontend/SwitchEmitter.cpp
@@ +229,5 @@
>              return false;
>          return true;
>      }
>  
> +    if (state_ == State::Case) {

Why is it okay to move this code? I don't think isDefault == true implies state_ != State::Case?

@@ +398,5 @@
>      }
>  
>      // Set the SRC_SWITCH note's offset operand to tell end of switch.
> +    // This code is shared between table switch and cond switch.
> +    MOZ_ASSERT(unsigned(SrcNote::TableSwitch::EndOffset) == unsigned(SrcNote::CondSwitch::EndOffset));

static_assert should work

	Jan de Mooij [:jandem]
	Comment 4 • 6 years ago

(In reply to Jan de Mooij [:jandem] from comment #3)
> Why is it okay to move this code? I don't think isDefault == true implies
> state_ != State::Case?

Oops, sorry, I missed your comment about this.

	Jan de Mooij [:jandem]
	Comment 5 • 6 years ago

One thing we've talked about before is to encode most source notes as bytecode immediates (except for the line/column number information; it would have to be stored separately).

However that's a bigger change and these patches really clean up and document what we have now, so I still think this is a good idea.

	Tooru Fujisawa [:arai] Assignee
	Comment 6 • 6 years ago

Separated the fix for SwitchEmitter's default case.
this bug was introduced by bug 1456006, where it wasn't adding source note for JSOP_DEFAULT, but I misunderstood it.

	Tooru Fujisawa [:arai] Assignee
	Comment 7 • 6 years ago

Added Count to enum, and replaced all consumers to use enum.

	Tooru Fujisawa [:arai] Assignee
	Comment 8 • 6 years ago

also added Count to enum

	Tooru Fujisawa [:arai] Assignee
	Comment 9 • 6 years ago

For now, I'm going to apply the same change for source notes which I'm not going to touch in other bugs (bug 1456404 etc)
here's the patch for SRC_COLSPAN.

the long line in BytecodeUtil-inl.h suggests that we should add static accessor method to the SrcNote::ColSpan class,
but I'd like to leave it to other bug, for simplicity.

	Tooru Fujisawa [:arai] Assignee
	Comment 10 • 6 years ago

same change for SRC_SETLINE.

	Jan de Mooij [:jandem]
	Comment 11 • 6 years ago

Comment on attachment 8994396 [details] [diff] [review]
Part 1: Add source note field constants for switch.

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

::: js/src/frontend/SourceNotes.h
@@ +43,5 @@
> +        enum Fields {
> +            // The offset of the end of switch (the first non-JumpTarget op
> +            // after switch) from JSOP_TABLESWITCH.
> +            EndOffset,
> +            Count,

Nit: I would remove the trailing comma after Count here and below. It normally makes sense: when you add a new value you don't have to add a comma to the previous line, but here we always want Count as last value I think :)

	Jan de Mooij [:jandem]
	Comment 12 • 6 years ago

Comment on attachment 8994398 [details] [diff] [review]
Part 3: Add source note field constants for colspan.

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

::: js/src/frontend/SourceNotes.h
@@ +86,5 @@
> +        enum Fields {
> +            // The column span (the diff between the column corresponds to the
> +            // current op and last known column).
> +            Span,
> +            Count,

We can use the Count value in the "table" below.

	Jan de Mooij [:jandem]
	Comment 13 • 6 years ago

Comment on attachment 8994399 [details] [diff] [review]
Part 4: Add source note field constants for line.

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

Beautiful.

	Tooru Fujisawa [:arai] Assignee
	Comment 14 • 6 years ago

https://hg.mozilla.org/integration/mozilla-inbound/rev/4f8e23bff248c7500302deb4b68804bbafbc462b
Bug 1477621 - Part 0: Remove unnecessary note from JSOP_DEFAULT. r=jandem

https://hg.mozilla.org/integration/mozilla-inbound/rev/5f7b22fe0124b332ea410e44b94afa11d29b254a
Bug 1477621 - Part 1: Add source note field constants for switch. r=jandem

https://hg.mozilla.org/integration/mozilla-inbound/rev/7cabc189feb9c32b870183b8096ca8702f518f61
Bug 1477621 - Part 2: Add source note field constants for try. r=jandem

https://hg.mozilla.org/integration/mozilla-inbound/rev/d18e67f75339c713ce1aadc54631898b5af5df3d
Bug 1477621 - Part 3: Add source note field constants for colspan. r=jandem

https://hg.mozilla.org/integration/mozilla-inbound/rev/9d328a1be52ce31ef56c4fe7e4155b4a6f2d35fc
Bug 1477621 - Part 4: Add source note field constants for line. r=jandem

	Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)
	Comment 15 • 6 years ago
bugherder

https://hg.mozilla.org/mozilla-central/rev/4f8e23bff248
https://hg.mozilla.org/mozilla-central/rev/5f7b22fe0124
https://hg.mozilla.org/mozilla-central/rev/7cabc189feb9
https://hg.mozilla.org/mozilla-central/rev/d18e67f75339
https://hg.mozilla.org/mozilla-central/rev/9d328a1be52c