Closed Bug 1477621 Opened Last year Closed Last year

Add constant or enum for source note offsets for each source note type.

Categories

(Core :: JavaScript Engine, enhancement, P3)

enhancement

Tracking

()

RESOLVED FIXED
mozilla63
Tracking Status
firefox63 --- fixed

People

(Reporter: arai, Assigned: arai)

References

Details

Attachments

(5 files, 2 obsolete files)

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.
Summary: Add constant or enum for source note offsets for each source note kind. → Add constant or enum for source note offsets for each source note type.
Blocks: 1456404
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)
Attachment #8994116 - Flags: feedback?(jdemooij)
also applied to try.
(simpler than Part 1)
Attachment #8994117 - Flags: feedback?(jdemooij)
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
Attachment #8994116 - Flags: feedback?(jdemooij) → feedback+
(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.
Attachment #8994117 - Flags: feedback?(jdemooij) → feedback+
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.
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.
Attachment #8994395 - Flags: review?(jdemooij)
Added Count to enum, and replaced all consumers to use enum.
Attachment #8994116 - Attachment is obsolete: true
Attachment #8994396 - Flags: review?(jdemooij)
also added Count to enum
Attachment #8994117 - Attachment is obsolete: true
Attachment #8994397 - Flags: review?(jdemooij)
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.
Attachment #8994398 - Flags: review?(jdemooij)
same change for SRC_SETLINE.
Attachment #8994399 - Flags: review?(jdemooij)
Attachment #8994395 - Flags: review?(jdemooij) → review+
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 :)
Attachment #8994396 - Flags: review?(jdemooij) → review+
Attachment #8994397 - Flags: review?(jdemooij) → review+
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.
Attachment #8994398 - Flags: review?(jdemooij) → review+
Comment on attachment 8994399 [details] [diff] [review]
Part 4: Add source note field constants for line.

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

Beautiful.
Attachment #8994399 - Flags: review?(jdemooij) → review+
You need to log in before you can comment on or make changes to this bug.