1148899 - Rename isExprClosure/setIsExprClosure/EXPR_CLOSURE.

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Tooru Fujisawa [:arai]]

EXPR_CLOSURE flag in JSFunction class is also used by arrow function with expression body (e.g. x => 1).
>        EXPR_CLOSURE     = 0x0020,  /* expression closure: function(x) x*x */

it should be better to rename it to something (EXPR_BODY?), to avoid confusing.

Comment 1

[Tooru Fujisawa [:arai]]

Attachment: Rename isExprClosure/setIsExprClosure/EXPR_CLOSURE to isExprBody/setIsExprBody/EXPR_BODY.

The flag could be removed if expression closure support is removed, but for now the name is misleading.

Green on try run: https://treeherder.mozilla.org/#/jobs?repo=try&revision=1687f3678938

Comment 2

[Shu-yu Guo [:shu]]

Comment on attachment 8585349 [details] [diff] [review]
Rename isExprClosure/setIsExprClosure/EXPR_CLOSURE to isExprBody/setIsExprBody/EXPR_BODY.

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

If we're going to rename anyways, how about isDeprecatedExprClosure? I imagine most SM hackers are familiar with what an "expr closure" is, despite the phrase itself being ambiguous. Prepending deprecated would make it clear that it's our own extension to the language that we want to phase out.

Comment 3

[Tooru Fujisawa [:arai]]

Oh, sorry if who confusing is only me,
does "expression closure" also contain arrow function with its body is an expression?
Currently the flags is also set to it.

Or perhaps what should be done here is avoid setting it to arrow function?

Comment 4

[Shu-yu Guo [:shu]]

(In reply to Tooru Fujisawa [:arai] from comment #3)
> Oh, sorry if who confusing is only me,
> does "expression closure" also contain arrow function with its body is an
> expression?
> Currently the flags is also set to it.
> 
> Or perhaps what should be done here is avoid setting it to arrow function?

I may have misunderstood. Is this bug really about whether or not the EXPR_CLOSURE flag should be set on arrow functions? I don't know the answer to that off the top of my head; let me get back to you tomorrow.

Comment 5

[Tooru Fujisawa [:arai]]

Let me clarify, the issue I thought is following:

  1. EXPR_CLOSURE flag is set to arrow function like `x => 1`
  2. "expression closure" means `function() 1`, not `x => 1` (is this correct?)

If 2 is true, 1 seems to be doing wrong (or at least misleading). so possible solutions would be following:

  A. rename EXPR_CLOSURE to EXPR_BODY (or something else, that contains both `x => 1` and `function() 1`)
  B. do not set EXPR_CLOSURE flag to arrow function

Comment 6

[Shu-yu Guo [:shu]]

(In reply to Tooru Fujisawa [:arai] from comment #5)
> Let me clarify, the issue I thought is following:
> 
>   1. EXPR_CLOSURE flag is set to arrow function like `x => 1`
>   2. "expression closure" means `function() 1`, not `x => 1` (is this
> correct?)
> 
> If 2 is true, 1 seems to be doing wrong (or at least misleading). so
> possible solutions would be following:
> 
>   A. rename EXPR_CLOSURE to EXPR_BODY (or something else, that contains both
> `x => 1` and `function() 1`)
>   B. do not set EXPR_CLOSURE flag to arrow function

Ah ha, thanks for the explanation. I'm fine with renaming EXPR_CLOSURE to EXPR_BODY to include the expression-bodied arrow functions.

Comment 7

[Shu-yu Guo [:shu]]

Comment on attachment 8585349 [details] [diff] [review]
Rename isExprClosure/setIsExprClosure/EXPR_CLOSURE to isExprBody/setIsExprBody/EXPR_BODY.

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

I had misunderstood the intent of the patch originally. LGTM after arai's explanation.

Comment 8

[Tooru Fujisawa [:arai]]

Thank you for reviewing :D

https://hg.mozilla.org/integration/mozilla-inbound/rev/e5c8e720814f

Comment 9

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/e5c8e720814f