svaarala / duktape

Duktape - embeddable Javascript engine with a focus on portability and compact footprint
MIT License
5.93k stars 514 forks source link

Deprecate DUK_DEFPROP_HAVE_XXX? #765

Open fatcerberus opened 8 years ago

fatcerberus commented 8 years ago

In Duktape 1.3.0 (or was it 1.4.0?), Duktape added the convenience flags DUK_DEFPROP_SET_XXX and DUK_DEFPROP_CLEAR_XXX. In light of these, the original DUK_DEFPROP_HAVE_XXX variants seem redundant. If you want to leave one of the flags as-is, you can just not pass a set or clear flag for it. The HAVE_XXX variants are thus not needed for the 3-way logic.

svaarala commented 8 years ago

Good point. There might be some cases where the flags are e.g. computed where the current value vs. have flag (basically a mask) might be useful.

fatcerberus commented 8 years ago

Actually one thing I would really like is convenience flags to set/clear multiple attributes:

DUK_DEFPROP_SET_EWC
DUK_DEFPROP_SET_EC
DUK_DEFPROP_CLEAR_EWC
etc.

It gets tedious typing out ENUMERABLE, CONFIGURABLE, WRITABLE over and over again and bloats any code that does a lot of duk_def_prop() calls, such as environment setup code.

Likewise, we could also have:

DUK_DEFPROP_VALUE
DUK_DEFPROP_GET
DUK_DEFPROP_SET
DUK_DEFPROP_GET_SET

Incidentally this is why I tend to avoid Object.defineProperty() when coding in JS - it's too verbose.

svaarala commented 8 years ago

Internally there are indeed DUK_PROPDESC_FLAGS_{W,E,C,WE,WC,WEC} for similar reasons. Adding similar constants seems fine to me.

Would DUK_DEFPROP_VALUE be the same as DUK_DEFPROP_HAVE_VALUE?

I agree Object.defineProperty() is verbose - but what would be the alternative, considering properties may get new attributes over time and an implementation might have custom attributes? Linear argument lists are very awkward for extensibility for example.

Since it's also a "slow path" API I think using an object to describe the requested change is actually best for API stability. This is also why I'd like to switch duk_def_prop() over to using an object argument (or perhaps a flag field + object argument combination) and to make it easier to work with objects to compensate for the change.

In general IMO object arguments to pass key/value arguments are great when: (1) the call is a "slow path" one (object creation etc), (2) the API is long-lived and difficult to change, (3) the API is likely to gain new features which can't be easily foreseen.

fatcerberus commented 8 years ago

Yeah, I meant DUK_DEFPROP_VALUE to be the same as _HAVE_VALUE.

Regarding the object argument: When setting multiple properties this would be even MORE inconvenient than using the flags unless Duktape keeps the object on the stack--which is not idiomatic for Duktape calls.

svaarala commented 8 years ago

Why would it be more inconvenient? For example, if there was an API call to push an object and define properties directly using a format string and a vararg list, there would be a single line to push a property descriptor (assuming no value or setter/getter was needed from value stack - but those are popped by duk_def_prop() as is).

svaarala commented 8 years ago

Re: idiomatic call styles, duk_def_prop() is actually different from practically all other calls in that there's a variable stack shape which is determined by flags rather than an argument count or similar. That's one reason I don't like the format of the call very much; it causes a lot of mistakes and questions when code is cut-and-pasted around. An object argument would be much more difficult to mess up with.

fatcerberus commented 8 years ago

If each duk_def_prop() call site (including the descriptor object) could be reduced to two lines of code, I'd be okay with that.

I knew you have that duk_def_prop_list() call in the offing, but I honestly thought that was what ultimately became of the property-format-string idea. So I was thinking in terms of that, which was still a bit more verbose than I would prefer. If you're still planning the printf-style object construction though, feel free ignore my protests. :)

fatcerberus commented 8 years ago

Oh, and by "idiomatic" I just meant it would be weird for a Duktape call to leave values on the valstack (even weirder than the variable stack shape). It was mostly just a stream of consciousness on my part--I was thinking of a hypothetical duk_def_prop() that left the descriptor on the stack, but then dismissed the idea as too radical.

svaarala commented 8 years ago

I'll try to put this in concrete terms. Consider modifying the property "foo" of an object so that:

Current call style:

duk_eval_string(ctx, "[1,2,3]");  /* whatever; push intended value */
duk_def_prop(ctx, obj_idx, DUK_DEFPROP_HAVE_VALUE |
                    DUK_DEFPROP_SET_WRITABLE |
                    DUK_DEFPROP_CLEAR_ENUMERABLE);

The same using an object style, assuming a "push and set properties" primitive (%S or something would indicate value is in value stack, and to be consumed):

duk_eval_string(ctx, "[1,2,3]");  /* whatever; push intended value */
duk_push_object_props(ctx, "value:%S writable:%b enumerable:%b", 1, 0);
duk_def_prop(ctx, obj_idx);  /* [ ... desc ] -> [ ... ] */
fatcerberus commented 8 years ago

Yeah, I figured out what you meant above, I just didn't know you were still planning duk_push_object_props(). I thought duk_def_prop_list() was what ultimately became of the idea, guess I was wrong :)

Also that duk_eval_string() is a neat trick. Why did I never think of that?

svaarala commented 8 years ago

duk_def_prop_list() is more a replacement for duk_put_function_list() and duk_put_number_list(), and is useful for module initialization especially which needs to be more convenient than it is now. But there's also a need to push objects inline without declaring an initializer array (which is awkward inline) and "printf style" is familiar to many users. While there's overlap between these calls, the needs are so common that it may be worth the footprint. Hopefully there are internal call sites which can also use the calls to offset the footprint - so far that's often been the case.

Returning to duk_def_prop(), there are internal call sites where the performance of that operation matters and they'd most likely need to keep on using a "raw" version of the call. I guess it might be that some user code has a similar need, so performance and avoiding memory churn created by an argument object are one of the counter arguments (in addition to already discussed above) to using an object argument.

svaarala commented 8 years ago

Using duk_eval_string() for stuff that needs to do be done in initialization is actually quite useful. It does create memory churn and is much slower than the equivalent C calls - but it's actually often smaller than the equivalent C code so it's often very useful in initialization.

One example is the minimal console provider which needs a Proxy wrapper which is setup like this:

    if (flags & DUK_CONSOLE_PROXY_WRAPPER) {
        duk_eval_string_noresult(ctx,
            "(function () {\n"
            "    var orig = console;\n"
            "    var dummy = function () {};\n"
            "    console = new Proxy(orig, {\n"
            "        get: function (targ, key, recv) {\n"
            "            var v = targ[key];\n"
            "            return typeof v === 'function' ? v : dummy;\n"
            "        }\n"
            "    });\n"
            "})();"
        );
    }
svaarala commented 7 years ago

Unassigning from 2.2.0 because defines can't be removed in minor versions (also in my view the lowest level defines should be kept because they may be necessary for computed masks).

svaarala commented 7 years ago

My preference would be still to make creating/parsing objects more convenient, and use objects as the base data type for all APIs where performance isn't critical and where extensions are most likely needed; duk_def_prop() would certainly be in that category so I hope to replace it with a more robust API in the future.