304828 - Array extras mislocated -- move to Function.prototype? Want Generics!

Status: VERIFIED FIXED

(Core :: JavaScript Engine, defect, P1)

Description

[Felipe]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.2; en-US; rv:1.7.10) Gecko/20050716 Firefox/1.0.6
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.2; en-US; rv:1.7.10) Gecko/20050716 Firefox/1.0.6

I wanted to file this before FF 1.5 was released. Array.prototype.map, while a
good idea, breaks standard OOP linguistic semantics and should be renamed before
much code is built on it.

Consider the following:
"Reverse this array."  -> someArray.reverse()
"Split this array using ','." -> someArray.split(/,/)
"Concatenate this array with another." -> someArray.concat(otherArray)
"Capitalize this string." -> someString.toUpperCase()

Notice that in each of these examples, the direct object of the sentence is the
object whose method is called, while any indirect object (or object of
preposition) is a parameter to the method.

So, following the same conventions, what should happen is:
"Map this function onto this array." -> someFunction.map(someArray);

someArray.map(someFunc), according to standard OOP linguistic semantics, says:
"Map this array onto this function."

Reproducible: Always

Steps to Reproduce:




I propose renaming Array.prototype.map to "process" or something similar. Then
it is:

"Process this array with this function." -> someArray.process(someFunc);

This is time-critical; the sooner it is resolved, the less code will be written
with the counter-intuitive naming. Certainly since there hasn't even been a
beta, it should be fine to rename this method.

Comment 1

[Josh Birnbaum]

->Core/js engine

Comment 2

[Blake Kaplan (:mrbkap) (inactive)]

I believe that the name "map" was taken from lisp roots, where one would call
(map someFun someArray). So in that sense, the name makes sense.

Comment 3

[Brendan Eich [:brendan]]

The argument is that "standard OOP linguistic semantics" require verb-named
methods to take the |this| parameter (self, the receiver, etc.) as the object of
the verb phrase, but map's object is the function.

In Lisp, Scheme, etc., map is a higher order function (function taking function
parameter) and so does not have a |this| parameter.  Indeed Lisp-hy map takes
the function to apply as its first parameter, and the list over which to map as
its second.  So Lisp ain't "standard OOP" -- good point.

I'm sympathetic to the bug, but not to the proposed new name.  "process" is way
too broad and vague.  For those in the know, "map" connotes the Lisp-ish meaning
we want, if you overlook the English-linguistic objection raised here.

Ideas for better names?  What do Python, Ruby, et al. call their higher order
map-like functions, if they are expressed as OOP-ish methods?

In the end, I think we'll be frustrated by making map be an Array method.  Would
we not be better off with a generic map global function that can operate on any
array-like object?  The problem then is global namespace pollution, but perhaps
we can solve that otherwise.

/be

Comment 4

[david]

How about moving map from Array.prototype to Function.prototype?

If you do this, though, I'd suggest that the thisObj argument would have to be
first to be parallel with call() and apply().

Similar arguments apply for the other Array Extras except indexOf and lastIndexOf.  
Shouldn't forEach(), filter(), some() and every() really be methods of Function?

And while I'm making crazy suggetions, how about an Array.prototype.addAll so that
a.addAll(b) works like Array.prototype.push.apply(a,b)

Comment 5

[Brendan Eich [:brendan]]

(In reply to comment #4)

> Similar arguments apply for the other Array Extras except indexOf and
lastIndexOf.  
> Shouldn't forEach(), filter(), some() and every() really be methods of Function?

YES!

/be

Comment 6

[Brendan Eich [:brendan]]

This is an API change we need to get right and document, now.  Since I failed to
catch this when reviewing, I'll take the bug.

David Flanagan: I'll defer addAll, noting that your use of apply is sufficient,
and wondering whether concat wouldn't be wanted more often (to preserve the two
arrays "in" parameters).

/be

Comment 7

[david]

Let me suggest that you reconsider the function names when you move them from
Array.prototype to Function.prototype.  In particular, I don't find some() and
every() very mnemonic.

Comment 8

[Brendan Eich [:brendan]]

some and every come from Common Lisp.  However, the current implementation does
not (because these are Array methods) deal with parallel processing of multiple
sequence arguments.  Moving these to Function.prototype and retaining the Common
Lisp names begs for that change, too.  Now we are far down the slippery slope!

/be

Comment 9

[Felipe]

As this bug gets discussed, please bear in mind the usefulness of such code as:

var formVals = document.getElementsByTagName('input')
.filter(function(i) { return (i.type == 'text'); })
.map(function(i) { return i.value; })
;

If anything does change in the current JS extras, it should be done to preserve
the ability to write highly concise, useful code like this.

Comment 10

[Felipe]

In response to the "some"/"every" naming question:

I thought "some" and "every" were funky names, too. What about "logicalOr" and
"logicalAnd", since that's literally what's being done? Highly intuitive and
easy to remember.

Comment 11

[shutdown]

(In reply to comment #3)
> Ideas for better names?  What do Python, Ruby, et al. call their higher order
> map-like functions, if they are expressed as OOP-ish methods?

In Ruby, Array class has the following instance methods:
grep (similar to Array.prototype.filter)
each (similar to Array.prototype.forEach)
map (similar to Array.prototype.map)

Comment 12

[Brendan Eich [:brendan]]

(In reply to comment #9)
> As this bug gets discussed, please bear in mind the usefulness of such code as:
> 
> var formVals = document.getElementsByTagName('input')
> .filter(function(i) { return (i.type == 'text'); })
> .map(function(i) { return i.value; })
> ;
> 
> If anything does change in the current JS extras, it should be done to preserve
> the ability to write highly concise, useful code like this.

How does that work today?  I see "undefined" alerted by this on the current bug
page: javascript:var t = document.getElementsByTagName('a');alert(t.forEach) --
and alert(t instanceof Array) in a similar URL shows "false".

It's easy to transpose operands from the inside out in what you wrote:

var formVals =
  (function (i) { return i.value; }).map(
    (function (i) { return i.type == 'text'; }).filter(
      document.getElementsByTagName('input')
    )
  )
;

although this is less clear, I agree -- but it's not less concise.

Sounds like Ruby commits the same verb/object solecism as the current Array
extras do.  OOP special-casing of |this| (self, whatever) is hurting us here. 
Generic functions would avoid that, and then you could write

var formVals = map(function(i){return i.value},
                   filter(function(i){return i.type == 'text'},
                          document.getElementsByTagName('input')));

Would this not be most concise and clear of all?

/be

Comment 13

[david]

If you're going to make them functions rather than methods (and I agree that
this might be an attractive option) how about making them properties of the
Function constructor.

So the example becomes:

var formVals = Function.map(function(i){return i.value},
                   Function.filter(function(i){return i.type == 'text'},
                          document.getElementsByTagName('input')));

This seems to improve clarity to me.  Although Function.filter might be better
as Array.filter...

Comment 14

[Brendan Eich [:brendan]]

So Function.prototype.apply set a bad precedent, and F.p.call is at best an
optimization:

f.call(thisObj, arg1, ... argN) === f.apply(thisObj, [arg1, ... argN])

In bootstrapping Narcissus (JS in JS,
http://lxr.mozilla.org/mozilla/source/js/narcissus/) we have wanted apply to be
an operator that composes with new, so one can invoke a constructor given an
argument list.

The some/every perplex also brings to mind operators such as Perl 6's junctions.

JS should not be a slave to the Java-ish OOP conventions it appropriated.

/be

Comment 15

[Brendan Eich [:brendan]]

(In reply to comment #13)

> var formVals = Function.map(function(i){return i.value},
>                    Function.filter(function(i){return i.type == 'text'},
>                           document.getElementsByTagName('input')));
> 
> This seems to improve clarity to me.  Although Function.filter might be better
> as Array.filter...

These are JS1 poor-man's namespaces, and mostly a matter of mnemonic taste and
good sense, but they make things quite a bit more verbose.  They do build on
existing precedent (String.fromCharCode, Date.parse).

I'm still looking for a solution that satisfies all goals, though, without too
much verbosity or OOP/FP mismatch.

/be

Comment 16

[Aiko]

I like the idea of moving these methods to Function.prototype. But more
importantly, I agree with comment 0: this is time critical. I would suggest that
you take your time to find the best solution but disable the methods immediately
to make it quite clear that they won't be included in their current form in
Firefox 1.5 (see the current mozillaZine front page).

Comment 17

[Felipe]

(In reply to comment #12)
> (In reply to comment #9)
> > As this bug gets discussed, please bear in mind the usefulness of such code as:
> > 
> > var formVals = document.getElementsByTagName('input')
> > .filter(function(i) { return (i.type == 'text'); })
> > .map(function(i) { return i.value; })
> > ;
> > 
> > If anything does change in the current JS extras, it should be done to preserve
> > the ability to write highly concise, useful code like this.
> 
> How does that work today?  I see "undefined" alerted by this on the current bug
> page: javascript:var t = document.getElementsByTagName('a');alert(t.forEach) --
> and alert(t instanceof Array) in a similar URL shows "false".
> 

I neglected to wrap my custom-made "makeArray" function around
getElementsByTagName in my example; excuse me.


> Sounds like Ruby commits the same verb/object solecism as the current Array
> extras do.  OOP special-casing of |this| (self, whatever) is hurting us here. 
> Generic functions would avoid that, and then you could write
> 
> var formVals = map(function(i){return i.value},
>                    filter(function(i){return i.type == 'text'},
>                           document.getElementsByTagName('input')));
> 
> Would this not be most concise and clear of all?
> 
> /be

I think "map" is too general a name for a global function. I propose making
"map" a Function method and coming up with a new name for the Array method (that
would just use the same code).

The real downside to a global function, besides namespace pollution, is that it
breaks the elegant OO "feel" of javascript. Even non-programmers are very used
now to thinking "object-oriented"; modern GUIs with right-click menus work the
same way as OO code: the menu shows a list of "properties" and "methods" for
whatever "object" you right-clicked on.

Re my suggestion of "process" - I agree that it's very general, and I've been
pondering what a better name might be. "iterate" could work, except that that
doesn't imply returning a "processed" array. Perhaps "transform"? "alter"?

The concept of starting with an array (as object) and ending with another one
is, I think, clearer and more intuitive than starting with a function and ending
with an array.

Comment 18

[Mike Shaver (:shaver emeritus)]

We should remember to tell these guys when we change everything around:

http://www.webreference.com/programming/javascript/ncz/column4/

Comment 19

[Felipe]

The extras were also reported on Slashdot....

Comment 20

[Brendan Eich [:brendan]]

Felipe, a some quick points:
- OOP is not always and everywhere elegant.  It's a style that can be used where
it doesn't make sense.
- I don't propose to put the name "map" into the global namespace, of course. 
See the end of comment 3 ("perhaps we can solve that otherwise").
- The fact that you had to make a DOM NodeList delegate to Array.prototype says
to me that most of the generic methods on Array (and on String) should be more
easy to use on any array-like object (one with length counting elements).

In JS2 there will be support for packages, which help isolate names of functions
from one another and put the user in charge of global (or other) name pollution.
 The current candidate syntax would require users to import from a package.
That's something we won't have implemented for Firefox 1.5, so whatever we do
will have to be kept around for backward compatibility.

I don't think we should withdraw these methods, since they provide much-wanted
functionality that's now actually used.

/be

Comment 21

[Brendan Eich [:brendan]]

An alternative to a package requiring an import is to put these functions in a
namespace, requiring either ::-qualified usage, or a use namespace statement. 
So the code in comment 9 would become either:

var ns = lisp_higher_order_functions;
var formVals = ns::map(function(i){return i.value},
                       ns::filter(function(i){return i.type == 'text'},
                                  document.getElementsByTagName('input')));

or

use namespace lisp_higher_order_functions;

var formVals = map(function(i){return i.value},
                   filter(function(i){return i.type == 'text'},
                          document.getElementsByTagName('input')));

I used an ugly_long_winded namespace name.  Suggestions for a better name, and
any comments at all, are welcome.

For 1.5, I'm tempted to use Function and Array as poor-man's namespaces as David
proposed.  Work on Edition 4 is about to take off and occupy much of my time,
and I'll make sure these methods get some attention, so we'll have to migrate
and keep compatibility no matter what we do.

If someone thinks we should just leave the methods in Array.prototype, please
argue against a quick cleanup patch for Firefox 1.5.

/be

Comment 22

[Brendan Eich [:brendan]]

All right, here is what I propose to do, in full, for Mozilla 1.8 / Firefox 1.5:

1.  Leave the Array extras where they are, but (conceptually -- actual layering
may be the other way around) reimplement them in terms of these new Array static
methods: every, filter, forEach, indexOf, lastIndexOf, map, and some.

2.  Factor generic "static methods" from the following *intentionally generic*
(search for that phrase in ECMA-262 Edition 3) methods of Array.prototype,
conceptually re-implementing the prototype methods in terms of their static
counterparts: concat, join, pop, push, reverse, shift, slice, sort, splice, and
unshift.

3.  Ditto for String.prototype generic methods: factor generic functions from
them provided as "static methods", effectively reimplementing the OOP forms in
terms of the new functions.  The generic String.prototype methods are: charAt,
charCodeAt, concat, indexOf, lastIndexOf, localeCompare, match, replace, search,
slice, split, substring, toLowerCase, toLocaleLowerCase, toUpperCase,
toLocaleUpperCase, and substr.

4.  Agonize over map being an Array method instead of Function.  Contemplate
adding redundant ways to call that favor the function argument over the
array-like argument both for OOP-ness and to use the better poor-man's namespace.

5.  Agonize over forEach vs. every -- wouldn't each go better with every (and
some; and make Ruby hackers' lives easier)?  I'm less perturbed by the Lisp-y
conciseness of every and some.

This may seem like overkill.  I argue that it's better to be consistent and make
all the *intentionally generic* prototype methods available as generic
functions, than to pick and choose.

One could argue (similar to my argument against doing addAll, see comment 6)
that Function.prototype.apply should be used to invoke generic methods on other
types of objects than the generic method's "home type", but (a) addAll has
distinctly different trade-offs, and (b) two wrongs don't make a right.

I think OOP-think led us away from generality and conciseness in making generic
methods instead of functions.  Selectively appealing to minimalism after this
non-minimal OOP-think mistake should not prevent us from fixing the mistake. 
And JS is non-minimal in its built-in methods anyway (consider slice, substring,
substr).

Comments?

/be

Comment 23

[Felipe]

I fail to see why anything but "map" would be relocated anyway. "filter"
particularly is in the right place: "Filter (elements of) this array using that
function." None of the other extras is a verb-named method, which was
established in comment 3 as being the issue at hand.

This being said, I'm all for renaming "every" and "some". These are hard names
to remember, IMO.

The idea of static methods seems to me a bit overy perl-ish in its
TMTOWTDI-ness. I'm not quite sure why one would *choose* to call
Array.join(anArray, glue) rather than anArray.join(glue); the former is wordier
and less clear. And since if it can happen it probably will, ultimately this
leads to JS being a language made harder to maintain by a greater number of ways
to get something done and longer documentation.

Comment 24

[Brendan Eich [:brendan]]

(In reply to comment #23)
> I fail to see why anything but "map" would be relocated anyway.

No relocation, generic-ization.

> "filter"
> particularly is in the right place: "Filter (elements of) this array using that
> function."

But it's in the wrong place if you want to operate on a DOM NodeList without
having to hack its prototype chain.

Hence the idea of static methods.

> None of the other extras is a verb-named method, which was
> established in comment 3 as being the issue at hand.

I morphed your bug, sorry.  I think you have only scratched the surface in
objecting to the map misnomer.  The mandatory OOP-ness is the underlying bug.

To make up for that morphing, how about this added proposal:

6. Rename map to collect (the original impetus for this bug was the map
misnomer).  See http://www.rubycentral.com/ref/ref_m_enumerable.html#collect for
Ruby precedent.

> This being said, I'm all for renaming "every" and "some". These are hard names
> to remember, IMO.

Harder than "each"?

> The idea of static methods seems to me a bit overy perl-ish in its
> TMTOWTDI-ness. I'm not quite sure why one would *choose* to call
> Array.join(anArray, glue) rather than anArray.join(glue);

Because anArray is not an instance of Array.

Remember, per ECMA-262, these are intentionally generic methods.

> the former is wordier
> and less clear.

Than what?  The relevant comparison is not what you showed, but the NodeList
counterexample you've hacked.  Why should everyone wanting to use one of these
generics on a NodeList, or on any array-like object (even a string!), have to
reinvent those hacks?

> And since if it can happen it probably will, ultimately this
> leads to JS being a language made harder to maintain by a greater number of
> ways to get something done and longer documentation.

Redundant forms does not make maintenance harder if they layer on a common
implementation, as they should.

/be

Comment 25

[Brendan Eich [:brendan]]

Ruby again: Array.map comes from Array including the Enumerable module, which is
generic.

I do not want to add modules or generics as a distinct type facility to JS1.x,
but I do wish to make allowed (see ECMA-262 Edition 3 Chapter 16) extensions
exposing generics without the mandatory-OOP-ness that requires ugly prototype
hacking or .apply usage.  And I definitely want to do this here, in this bug,
rather than just rename map or move generics from one relatively narrow
prototype to another.

/be

Comment 26

[Felipe]

> > "filter"
> > particularly is in the right place: "Filter (elements of) this array using that
> > function."
> 
> But it's in the wrong place if you want to operate on a DOM NodeList without
> having to hack its prototype chain.

AHHHH....ok, this just made sense. Yes, not having to do makeArray(someNodeList)
would be nice.


> 6. Rename map to collect (the original impetus for this bug was the map
> misnomer).  See http://www.rubycentral.com/ref/ref_m_enumerable.html#collect for
> Ruby precedent.

How does "collect" describe mapping a function onto an array?

> 
> > This being said, I'm all for renaming "every" and "some". These are hard names
> > to remember, IMO.
> 
> Harder than "each"?

Well, harder than "logicalOr" and "logicalAnd". :) "each" is worse yet, since
that's usually used as an iterator.

Comment 27

[Brendan Eich [:brendan]]

(In reply to comment #26)
> How does "collect" describe mapping a function onto an array?

Since this is arr.prototype.collect(fun) or Array.collect(arrlike, fun), the
verb and object are "collect" and "function-mapped values" (in a new array).

It's all about the object of the verb: you map a function over an array, or you
collect mapped values from an array into another array.

> > > This being said, I'm all for renaming "every" and "some". These are hard
> > > names to remember, IMO.
> > 
> > Harder than "each"?
> 
> Well, harder than "logicalOr" and "logicalAnd". :) "each" is worse yet, since
> that's usually used as an iterator.

I didn't mean to propose each as another name for every or some, only to compare
it (shaver chose forEach, but each matches every nicely).  It's true that every
and some are somewhat more terse than the Java-influenced method names in JS,
but logicalAnd and logicalOr are long without differing except in the last two
or three chars.  Still looking for better names.

/be

Comment 28

[Mike Shaver (:shaver emeritus)]

At the time, I considered names like

visit for forEach
allMatch for every
anyMatch for some

visit seemed too computer-science, but it has the virtue of not colliding with
E4X's new "for each" construction.  (I think it is similar to "for each" in
behaviour, though, so maybe that's a good collision.  I also considered a
tryEach variant that swallowed exceptions thrown or propagated by the provided
function, because I found myself using that construct a lot when doing things
like calling fallible observers, the failure of one of which should not prevent
others from being called.)

allMatch and anyMatch were mainly considered because there was a fair bit of
conceptual distance between "every" and "every one of the elements in this array
will product a true value when passed to this function".  But I didn't like
"match" much better than the implication, so I followed Lisp.

I actually considered making "map" a member of Function.prototype, but decided
that it was close enough to Array.prototype.sort ("sort this array according to
the provided function") or even String.prototype.split ("split this array
according to the provided regexp").  I wish I could find the IRC conversation I
had with someone (vlad?) about this, to recount here.

I disagree with the reasoning in comment 0, perhaps predictably: "map this array
according to the provided function" seems more natural an interpretation to me,
given that we don't concat this array _onto_ the provided one, or split the
string _onto_ the regexp.  We perform the operation named by the method _with_
the argument, or according to the rules specified by it (in the split case).  I
think you really do map a set (array), and that the function is a detail, but
maybe that's just because I see the data as primary in this operation, and not
the function. I would reason on the basis of starting with one set of data and
wanting another, and construct the function to perform the transformation I
need, rather than having a function and finding a set to map with it.  (Again,
analogous to sorting an array, or splitting a string: the rules for the
operation are secondary to the object on which the operation is performed, and
therefore the object gets to be the "subject", the honoured LHS of the .-operator.)

I'm out sick today, or at least until this maybe-food-poisoning clears, but I'll
try to watch this bug for action.

Comment 29

[david]

In response to comment #26, I want to withdraw the objection I expressed in
comment #7 to the names "every" and "some".  I have since realized that these
functions are intended for use as the condition of an if statement or while
loop, and in that context, they are clear and concise.  Much clearer, actually
than logicalAnd() and logicalOr().

Using the generic version Brendan is now proposing, I could write this:

if (Array.every(array, predicate)) 

this tests if every element in the array satisfies the predicate.

And to see if any elements in the array satisfy the predicate:

if (Array.some(array, predicate))

If anything any() might be better than some().

In any case, now that I've figured out how these functions are intended to be
used, I think they are well-named.

Comment 30

[Brendan Eich [:brendan]]

The patch will be easy to do, but the issue needs to be resolved to the
satisfaction of crucial JS landholders, at least within the Mozilla world if not
in ECMA TG1.  Again, we can carry extensions to ECMA forever, if they are cheap
to maintain (as these will be).

/be

Comment 31

[Brendan Eich [:brendan]]

(In reply to comment #29)
> Using the generic version Brendan is now proposing, I could write this:
> 
> if (Array.every(array, predicate)) 
> 
> this tests if every element in the array satisfies the predicate.

Again, just to be super-clear, I want to leave the Array.prototype OOP methods
too, so if array above satisfies (array instanceof Array), that could be better
written:

if (array.every(predicate))

Acknowledged that any is slightly better than some, but (a) Lisp precedent helps
slightly in the other direction; (b) any is a Perl 6 junction and I'd like not
to use that name.

/be

Comment 32

[Mike Shaver (:shaver emeritus)]

The .NET Generics collection has:

List.TrueForAll(array, predicate)

http://msdn2.microsoft.com/library/kdxe4x4w.aspx

(Array isa List)

Comment 33

[Brendan Eich [:brendan]]

Attachment: Patch as promised

No collect synonym, no forEach=>each renaming, just static method entry points
for the generic prototype methods that take the |this| param of the prototype
method as the first param, and shift the other params (if any) over one
position.

Footprint hit on gcc 3.4.4 (FC3): 4512 bytes.  A single native that dispatches
based on name could save more space, at the cost of runtime.  It would add some
space, so the savings would be < 4K.  Depending on how it dispatches, it might
actually lose.	Anyway, I think 4K is acceptable.  Anyone disagree?

I'm seriously considering David suggestion via email to bump our "JS version"
number to 1.6.

/be

Comment 34

[Mike Shaver (:shaver emeritus)]

Comment on attachment 193868 [details] [diff] [review]
Patch as promised

sr=shaver, thanks for taking this.

Comment 35

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 193868 [details] [diff] [review]
Patch as promised

>Index: jsstr.c
>+    {"fromCharCode",    str_fromCharCode,                 1,0,0},
>+
>+#if JS_HAS_TOSOURCE
>+    {"quote",               str_static_quote,             1,0,0},
>+#endif

str_fromCharCode doesn't line up correctly.

r=mrbkap with that nit picked.

Comment 36

[Brendan Eich [:brendan]]

Attachment: patch for smaller footprint increase, via generic native method dispatcher

Shaved down to a 1434-byte increase:

$ diff {old,newer}sz
2c2
<  546066	  16796     240  563102   8979e libmozjs.so
---
>  547048	  17248     240  564536   89d38 libmozjs.so
$ cat newersz
   text    data     bss     dec     hex filename
 547048   17248     240  564536   89d38 libmozjs.so

/be

Comment 37

[Mike Shaver (:shaver emeritus)]

Comment on attachment 194118 [details] [diff] [review]
patch for smaller footprint increase, via generic native method dispatcher

Is it safe to presume that the JSFunctionSpec will live after we return from
JS_DefineGenericFunctions?  We don't require that for JS_DefineFunctions, so
people might find it surprising.  (Though we do require that fs->extra be
appropriately long-lived.)

We should at least document that requirement in jsapi.h.  Embedders who can't
live with it can roll their own.

sr=shaver with addition to jsapi.h's comment.

Comment 38

[Brendan Eich [:brendan]]

Comment updated:

+/*
+ * Define generic, arity N+1 methods on the class constructor whose prototype
+ * is obj, based on their arity N native counterparts specified by fs.
+ *
+ * E.g., to expose a generic Array.prototype.join method, whose native
function+ * is array_join, as an Array.join static method, include
+ *
+ *    {"join", array_join, 2, 0, 0}
+ *
+ * in fs, and pass Array.prototype (or any instance delegating to it) as obj.
+ * This is done for all generic Array and String methods, to relieve callers
+ * who pass non-instance objects that conform to the generics' requirements
+ * from having to use Function.prototype.apply or .call.
+ */
+extern JS_PUBLIC_API(JSBool)
+JS_DefineGenericMethods(JSContext *cx, JSObject *obj, JSFunctionSpec *fs);

/be

Comment 39

[Mike Shaver (:shaver emeritus)]

I'm having a dumb spell: how does that comment indicate the lifetime
requirements for fs?

Comment 40

[Brendan Eich [:brendan]]

Oops, copied and pasted from an old diff.  Here's the latest:

+/*
+ * Define generic, arity N+1 methods on the class constructor whose prototype
+ * is obj, based on their arity N native counterparts specified by fs.
+ *
+ * E.g., to expose a generic Array.prototype.join method, whose native
function+ * is array_join, as an Array.join static method, include
+ *
+ *    {"join", array_join, 2, 0, 0}
+ *
+ * in fs, and pass Array.prototype (or any instance delegating to it) as obj.
+ * This is done for all generic Array and String methods, to relieve callers
+ * who pass non-instance objects that conform to the generics' requirements
+ * from having to use Function.prototype.apply or .call.
+ *
+ * NB: The storage at fs must live as long as these methods live; typically
+ * fs is a static, initialized array of JSFunctionSpec.
+ */
+extern JS_PUBLIC_API(JSBool)
+JS_DefineGenericMethods(JSContext *cx, JSObject *obj, JSFunctionSpec *fs);

/be

Comment 41

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 194118 [details] [diff] [review]
patch for smaller footprint increase, via generic native method dispatcher

r=mrbkap

Comment 42

[shutdown]

(In reply to comment #36)
> Created an attachment (id=194118) [edit]
> patch for smaller footprint increase, via generic native method dispatcher

> +    /*
> +     * Copy args down over our |this| parameter, argv[-1], which is likely
> +     * the class constructor object, e.g. Array.  Then call the prototype
> +     * native method with our first argument passed as |this|.
> +     */
> +    memmove(argv - 1, argv, argc * sizeof(jsval));
> +    return fs->call(cx, JSVAL_TO_OBJECT(argv[-1]), argc - 1, argv, rval);

Integer underflow happens if argc == 0.
What should be done if generics are called with no args?

Comment 43

[Brendan Eich [:brendan]]

Oops.  Thanks, based on shutdown's comment I found my brain (I accidentally left
it in the diaper pail at 4:30am this morning -- colicky newborn, what can I
say!). I'll fix the generic dispatcher to deal with too-few args.

/be

Comment 44

[Brendan Eich [:brendan]]

Attachment: interdiff against last patch to fix the generic dispatcher

This fixes not just the bug shutdown pointed out, which took some thinking cuz
we want to follow ECMA-262 Edition 3 on Function.prototype.apply/.call, but
also the bug where missing but required args (fs->nargs - argc counts these,
off the end of the argc actual args passed in argv -- the JS engine guarantees
that these slots in argv are valid) were not copied down one jsval.

Re-requesting reviews.

/be

Comment 45

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 194230 [details] [diff] [review]
interdiff against last patch to fix the generic dispatcher

>+    if (argc == 0) {
> ...
>+        argv[0] = OBJECT_TO_JSVAL(obj);

If fs->nargs is also 0, then this could overwrite the end of the argv array. We
should probably do something a bit better. Brendan says that he'll fix this
when he checks in, so r=mrbkap.

Comment 46

[Brendan Eich [:brendan]]

Attachment: best patch yet!

Down to a 432 byte increase.  This retasks JSFUN_LAMBDA, an internal flag used
only for scripted functions, as JSFUN_GENERIC_NATIVE when used in the flags
member of JSFunctionSpec.  This is a backward-compatible API extension, since
JSFUN_LAMBDA was meaningless on natives till now (except to the decompiler,
where it would add unwanted parenthesization).

$ cat newestsz
   text    data     bss     dec     hex filename
 546498   16796     240  563534   8994e libmozjs.so
$ diff {old,newest}sz
2c2
<  546066	  16796     240  563102   8979e libmozjs.so
---
>  546498	  16796     240  563534   8994e libmozjs.so

This bug is an exercise in proving myself wrong by taking the time to minimize.


/be

Comment 47

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 194256 [details] [diff] [review]
best patch yet!

r=mrbkap

Comment 48

[shutdown]

(In reply to comment #46)
> Created an attachment (id=194256) [edit]
> best patch yet!

> +            /* We can't assume fs->call is well-aligned, so store fs instead. */
> +            if (!JS_SetReservedSlot(cx, fun->object, 0, PRIVATE_TO_JSVAL(fs)))
> +                return JS_FALSE;

This comment is not needed anymore since js_generic_native_method_dispatcher()
uses not only fs->call but also fs->nargs now.

Comment 49

[Brendan Eich [:brendan]]

(In reply to comment #48)
> 
> This comment is not needed anymore since js_generic_native_method_dispatcher()
> uses not only fs->call but also fs->nargs now.

Thanks, good point!  I'll fix.

/be

Comment 50

[Mike Shaver (:shaver emeritus)]

Comment on attachment 194256 [details] [diff] [review]
best patch yet!

Best yet! sr=shaver, thanks to shutdown for his catch.

Comment 51

[Brendan Eich [:brendan]]

Fixed on trunk and branch.

Need to document these on DevMo.  Do we file bugs for such tasks?

Do these, plus the Array extras, add up to "JS1.6"?  "JS1.5.1"?  Comments welcome.

/be

Comment 52

[Mike Shaver (:shaver emeritus)]

Comment on attachment 194230 [details] [diff] [review]
interdiff against last patch to fix the generic dispatcher

sr=shaver

Comment 53

[Bob Clary [:bc] (inactive)]

Checking in regress-304828.js;
/cvsroot/mozilla/js/tests/js1_6/Array/regress-304828.js,v  <--  regress-304828.js
initial revision: 1.1

Kind of funky behavior using the Array methods on Strings. Please check this test to make sure it is expecting the correct behavior.

Comment 54

[Bob Clary [:bc] (inactive)]

verified fixed 1.8.x and trunk.