Closed Bug 988903 Opened 6 years ago Closed 6 years ago

Clean-up Snapshot constant naming and bit packing.

Categories

(Core :: JavaScript Engine: JIT, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla31

People

(Reporter: nbp, Assigned: nbp)

References

Details

Attachments

(1 file)

No description provided.
Attachment #8397872 - Flags: review?(kvijayan)
Comment on attachment 8397872 [details] [diff] [review]
Rename Snapshots packing constants.

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

::: js/src/jit/Snapshots.cpp
@@ +476,5 @@
> +static const uint32_t SNAPSHOT_RESUMEAFTER_MASK = COMPUTE_MASK(SNAPSHOT_RESUMEAFTER);
> +
> +static const uint32_t SNAPSHOT_FRAMECOUNT_SHIFT = COMPUTE_SHIFT(SNAPSHOT_RESUMEAFTER);
> +static const uint32_t SNAPSHOT_FRAMECOUNT_BITS = 32 - 4;
> +static const uint32_t SNAPSHOT_FRAMECOUNT_MASK = COMPUTE_MASK(FRAMECOUNT);

I fixed this typo locally: FRAMECOUNT -> SNAPSHOT_FRAMECOUNT (and compiling and checking fine)
Assignee: nobody → nicolas.b.pierron
Status: NEW → ASSIGNED
Comment on attachment 8397872 [details] [diff] [review]
Rename Snapshots packing constants.

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

::: js/src/jit/Snapshots.cpp
@@ +20,5 @@
>  
>  // Snapshot header:
>  //
> +//   [vwu] bits ((n+1)-31]: frame count
> +//         bits n+1: resume after

Nit: should be "bit n+1", not "bits n+1".

@@ +464,5 @@
>      readSnapshotHeader();
>  }
>  
> +#define COMPUTE_SHIFT(name) (name ## _BITS + name ##_SHIFT)
> +#define COMPUTE_MASK(name) (((1 << name ## _BITS) - 1) << name ##_SHIFT)

Temporary #defines like this should be suffixed with underscore:

COMPUTE_SHIFT_
COMPUTE_MASK_

Also, COMPUTE_SHIFT_ is better named COMPUTE_SHIFT_ABOVE_ or COMPUTE_SHIFT_AFTER_.

When it's used below, the expression "COMPUTE_SHIFT(SNAPSHOT_BAILOUTKIND)" seems to imply you're computing the shift for BAILOUTKIND, not for the bitfield after BAILOUTKIND.
Attachment #8397872 - Flags: review?(kvijayan) → review+
https://hg.mozilla.org/mozilla-central/rev/2defc196c2d2
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla31
You need to log in before you can comment on or make changes to this bug.