clean up the JSFUN_* flags situation

RESOLVED FIXED in mozilla19

Status

()

defect
RESOLVED FIXED
7 years ago
6 years ago

People

(Reporter: luke, Unassigned)

Tracking

(Blocks 1 bug)

unspecified
mozilla19
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

Reporter

Description

7 years ago
Posted patch patchSplinter Review
Right now it has hard to understand what function flags are claimed and which are free.  One source of problems is that jsapi.h introduces one "flag space" of flags that are specified in JSFunctionSpec whereas JSFunction::flags has a different, partially overlapping space.  This patch separates these concerns, unpacks the bits a bit (we still have 4 free JSFunction::flags bits after), and hides a bunch of flags that shouldn't have been exposing in jsapi.h in the first place.

Also, we are totally getting bits mixed up: fun->isSelfHostedConstructor() is getting set in some cases for natives and, since JSFUN_FLAGS_MASK overlays JSPROP_SHARED we were, effectively masking it in calls to defineProperty.  (This is why I had to remove JSPROP_SHARED from jsperf.cpp.)
Attachment #676910 - Flags: review?(jorendorff)
Comment on attachment 676910 [details] [diff] [review]
patch

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

Most excellent! The flags situation has been irking me quite a bit, thanks for cleaning it up.

I wonder if we should also clean up the CTOR stuff: right now, we've got 3 different is*Constructor predicates, where ideally we'd just have isConstructor. But see my question about JSFUN_CONSTRUCTOR below.

::: js/src/jsapi.h
@@ +2531,4 @@
>                                             instead of defaulting to class gsops
>                                             for property holding function */
>  
> +#define JSFUN_CONSTRUCTOR      0x400    /* native that can be called as a ctor */

The "without creating a this object" part seems important to me. Is it not?
Reporter

Comment 3

7 years ago
(In reply to Till Schneidereit [:till] from comment #2)
> I wonder if we should also clean up the CTOR stuff: right now, we've got 3
> different is*Constructor predicates, where ideally we'd just have
> isConstructor. But see my question about JSFUN_CONSTRUCTOR below.

Yeah, I hadn't been thinking deeply about the flags themselves but you're right.  Maybe we can do that in a followup bug?

> > +#define JSFUN_CONSTRUCTOR      0x400    /* native that can be called as a ctor */
> 
> The "without creating a this object" part seems important to me. Is it not?

The reason I removed it is that, IIUC, we *never* create 'this': interpreted functions do it in StackFrame::prologue, natives are either isNativeConstructor or throw.  See also JS_IsConstructing and JS_NewObjectForConstructor and comments.
(In reply to Luke Wagner [:luke] from comment #3)
> (In reply to Till Schneidereit [:till] from comment #2)
> > I wonder if we should also clean up the CTOR stuff: right now, we've got 3
> > different is*Constructor predicates, where ideally we'd just have
> > isConstructor. But see my question about JSFUN_CONSTRUCTOR below.
> 
> Yeah, I hadn't been thinking deeply about the flags themselves but you're
> right.  Maybe we can do that in a followup bug?

Of course. I just saw that cleaning that up would make sense, too.
> 
> > > +#define JSFUN_CONSTRUCTOR      0x400    /* native that can be called as a ctor */
> > 
> > The "without creating a this object" part seems important to me. Is it not?
> 
> The reason I removed it is that, IIUC, we *never* create 'this': interpreted
> functions do it in StackFrame::prologue, natives are either
> isNativeConstructor or throw.  See also JS_IsConstructing and
> JS_NewObjectForConstructor and comments.

Ah, I see. That clears up some questions I had about the construction sequence, thanks.
Reporter

Updated

7 years ago
Blocks: 807401
Reporter

Updated

7 years ago
Blocks: 807185
Comment on attachment 676910 [details] [diff] [review]
patch

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

Very nice. Thanks!

::: js/src/frontend/Parser.cpp
@@ +1059,2 @@
>      if (selfHostingMode)
> +        fun->setIsSelfHostedBuiltin();

You tweaked the order of these two things; but I liked it a bit better the other way round, because then there's one less mutating operation on functions.

(Not worth mentioning perhaps.)

::: js/src/jsfun.cpp
@@ +425,5 @@
>          return false;
>  
>      if (mode == XDR_DECODE) {
>          fun->nargs = flagsword >> 16;
> +        fun->flags = flagsword;

Isn't this still better with a cast, since flagsword is u32?

@@ +598,5 @@
>      TokenKind tt = ts.getToken();
>      if (tt == TOK_ERROR)
>          return false;
>      bool braced = tt == TOK_LC;
> +    JS_ASSERT(fun->isExprClosure() ^ braced);

Random bikeshed: While you're here: "== !braced" might be a bit nicer than "^ braced"? ...but that's pretty subjective, of course...

@@ +757,5 @@
>          if ((!bodyOnly && !out.append("() {\n    ")) ||
>              !out.append("[sourceless code]") ||
>              (!bodyOnly && !out.append("\n}")))
>              return NULL;
> +        if (!lambdaParen && fun->isLambda() && (!out.append(")")))

I'm not sure what those extra parens are doing there; feel free to get rid of them.

::: js/src/jsfun.h
@@ +30,5 @@
> +        IS_FUN_PROTO       = 0x0010,  /* function is Function.prototype for some global object */
> +        EXPR_CLOSURE       = 0x0020,  /* expression closure: function(x) x*x */
> +        HAS_GUESSED_ATOM   = 0x0040,  /* function had no explicit name, but a
> +                                         name was guessed for it anyway */
> +        LAMBDA             = 0x0080,  /* function expression (not a statement) */

While you're renaming, maybe EXPRESSION?

The comment is OK but could be more precise:
    /* a FunctionExpression (not a declaration) */
or more verbosely:
    /* function comes from a FunctionExpression or Function() call
       (not a FunctionDeclaration or nonstandard function-statement) */

@@ +40,5 @@
> +        HAS_DEFAULTS       = 0x0800,  /* function has at least one default parameter */
> +
> +        /* Derived Flags values for convenience: */
> +        NATIVE_FUN         = 0,
> +        LAMBDA_FUN         = LAMBDA | INTERPRETED

Kind of a gross name, since it's not obvious what distinguishes LAMBDA_FUN from LAMBDA.  LAMBDA_INTERPRETED perhaps.

@@ +266,5 @@
>  
>  extern JSString *
>  fun_toStringHelper(JSContext *cx, js::HandleObject obj, unsigned indent);
>  
> +static inline JSFunction::Flags

As far as I'm concerned, since we no longer support C clients at all, you can drop "static" here.

::: js/src/perf/jsperf.cpp
@@ +89,5 @@
>      JS_SET_RVAL(cx, vp, BOOLEAN_TO_JSVAL(p->canMeasureSomething()));
>      return JS_TRUE;
>  }
>  
> +const uint8_t PM_FATTRS = JSPROP_READONLY | JSPROP_PERMANENT;

I think removing JSPROP_SHARED is definitely the right thing here; I'm kind of astonished it worked without it.

@@ +99,5 @@
>      JS_FS_END
>  };
>  
>  const uint8_t PM_PATTRS =
> +    JSPROP_ENUMERATE | JSPROP_READONLY | JSPROP_PERMANENT;

I think in this context, JSPROP_SHARED is appropriate. It just means the prototype properties are slotless. It might be better to convert these properties to real accessor properties, a la JS_PSG, but that's more work.
Attachment #676910 - Flags: review?(jorendorff) → review+
Reporter

Comment 6

7 years ago
(In reply to Jason Orendorff [:jorendorff] from comment #5)
> You tweaked the order of these two things; but I liked it a bit better the
> other way round, because then there's one less mutating operation on
> functions.

Since 'flags' is an enum (as required by js_NewFunction), you'd need:
  flags = JSFunction::Flags(flags | JSFunction::SELF_HOSTED);
which seemed a bit gross.  In general I avoided these casts by moving them into the JSFunction helpers.  I could go the other way if you still had a preference.

> Isn't this still better with a cast, since flagsword is u32?

Oops, yes.  That change is leftover from when I had JSFunction::flags as an enum, rather than a uint16_t.  (That ended up being more trouble than it was worth, due to the need for a bitfield.)

> > +        LAMBDA             = 0x0080,  /* function expression (not a statement) */
> 
> While you're renaming, maybe EXPRESSION?

I kinda like LAMBDA and none of "isExpression" and "isFunExpr" or "isFunctionExpression" sound that intuitive without reading the comment.  Perhaps we can leave LAMBDA as-is for now and rename it later?

The rest of the suggestions sound great, thanks!
https://hg.mozilla.org/mozilla-central/rev/247fb0614615
Status: NEW → RESOLVED
Last Resolved: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla19
Depends on: 837192
No longer depends on: 837192
You need to log in before you can comment on or make changes to this bug.