+1; looks good overall. I’m fine with this being committed as it is. Few somewhat unrelated thoughts that came to me as I was reviewing this:
- do we need toUint32 to return long anymore? Could it just return double? There’s a place in the patch where we’re casting it. - NashornPrimitiveLinker.canLinkTypeStatic: what about ES6 symbols? They’re primitive values too, right? A primitive symbol value can have e.g. toString() invoked on it (or in a more absurd case, any additional method added to Symbol.prototype). Would that work similarly to how we can invoke methods on other primitive values? Attila. > On Jan 12, 2016, at 10:33 AM, Hannes Wallnoefer > <hannes.wallnoe...@oracle.com> wrote: > > I uploaded a new webrev without the changes to the parser API. > > http://cr.openjdk.java.net/~hannesw/8143896/webrev.03/ > > Note that parserapi.js.EXPECTED changes because of the changes in > parserapi.js, which is itself included in the files it parses. > > Please review. > > Hannes > > Am 2016-01-11 um 16:57 schrieb Sundararajan Athijegannathan: >> As discussed offline, please leave Nashorn Parser API changes for a separate >> issue. >> >> -Sundar >> >> On 1/11/2016 8:07 PM, Hannes Wallnoefer wrote: >>> I fixed a bug with converstion to number for the strict equality operator, >>> which also revealed some left over usage of long in Nashorn internals. New >>> webrev is here: >>> >>> http://cr.openjdk.java.net/~hannesw/8143896/webrev.02/ >>> >>> Hannes >>> >>> Am 2016-01-11 um 13:48 schrieb Hannes Wallnoefer: >>>> You are right of course, there needs to be consistency between typeof >>>> operator and treatment as JS numbers. >>>> >>>> This is in fact an unpleasant problem to solve. I've struggled trying to >>>> fix this without breaking any existing code, but I've come to the >>>> conclusion that it is not possible. Since we can't treat all Java >>>> longs/Longs as JS numbers, we'd have to differentiate depending on whether >>>> the value can be represented as double without losing precision. >>>> >>>> In a way we already do this with optimistic types, but I consider it more >>>> a bug than a feature. It's weird (and error prone) if the return value for >>>> a Java method returning long is reported as number or object depending on >>>> the actual value. >>>> >>>> So I think the right thing to do is draw a clear line between which Java >>>> primitive/wrapper types represent JS numbers and which don't. I've >>>> uploaded a new webrev that implements this: >>>> >>>> http://cr.openjdk.java.net/~hannesw/8143896/webrev.01/ >>>> >>>> Note that the only types to be treated as JS numbers are the direct >>>> wrapper classes for Java primitives that can be fully represented as >>>> doubles. This means also things like AtomicInteger and DoubleAdder will be >>>> reported and treated as objects. I think that's the correct thing to do as >>>> they are not primitive numbers in the first place. They are still >>>> converted to numbers when used in such a context in JS. So I think the >>>> only place where this change is a actually painful/surprising is longs. >>>> >>>> Unfortunately the check for number type in JSType.isNumber gets a bit long >>>> as we have to individually check for all primitive wrapper classes. I've >>>> done extensive benchmarking and I don't think it has an impact on >>>> performance. In any way, I wouldn't know how to handle this differently. >>>> >>>> Let me know what you think. >>>> >>>> Hannes >>>> >>>> Am 2016-01-04 um 05:00 schrieb Sundararajan Athijegannathan: >>>>> I think I already commented on this webrev -- that we need to cover tests >>>>> for BigInteger, BigDecimal. >>>>> >>>>> Also, I'm not sure linking Double and Int by nashorn primitive linkers is >>>>> the right solution. AtomicInteger, DoubleAdder etc. are all Number >>>>> subtypes. We return "number" when typeof is used on any Number subtype. >>>>> Now, that means JS code will see these as 'number' type objects -- yet >>>>> Number.prototype methods won't work on those!! I know this is hard >>>>> problem -- we also have another (somewhat related) BigDecimal, BigInteger >>>>> toString / String conversion issue. We need to discuss this. >>>>> >>>>> -Sundar >>>>> >>>>> On 1/2/2016 8:29 PM, Attila Szegedi wrote: >>>>>> +1 >>>>>> >>>>>>> On Dec 18, 2015, at 3:54 PM, Hannes Wallnoefer >>>>>>> <hannes.wallnoe...@oracle.com> wrote: >>>>>>> >>>>>>> Please review JDK-8143896: java.lang.Long is implicitly converted to >>>>>>> double >>>>>>> >>>>>>> http://cr.openjdk.java.net/~hannesw/8143896/webrev/ >>>>>>> >>>>>>> Thanks, >>>>>>> Hannes >>>>> >>>> >>> >> >
