Questions about WrapFactory and javaPrimitiveWrap

[tonygermano]

At first glance I was going to suggest a naive approach to eliminating most of the instanceof checks in the wrap method and replacing them with instanceof Number.

rhino/src/org/mozilla/javascript/WrapFactory.java

Lines 56 to 68 in 3e205d7

	if (!isJavaPrimitiveWrap()) {
	if (obj instanceof String
	|| obj instanceof Boolean
	|| obj instanceof Integer
	|| obj instanceof Short
	|| obj instanceof Long
	|| obj instanceof Float
	|| obj instanceof Double
	|| obj instanceof BigInteger) {
	return obj;
	} else if (obj instanceof Character) {
	return String.valueOf(((Character) obj).charValue());
	}

I then noticed that Byte is not currently covered.

When I looked at other Number implementations I found AtomicXXX versions and BigDecimal.

So, the questions...

1. Should we include Byte? I think yes.
2. Should we include AtomicXXX? I think no, due to loss of atomicity.
3. Should we include BigDecimal? I think no, due to potential loss of precision. I would have said the same about BigInteger, except now we have proper support.
   3b. Should we retain Long? Tough call here since it also has potential for loss of precision. Maybe another flag for this?

[tonygermano]

I just looked at the history of that file, and, in fact, it used to be a test for Number and was changed due to BigInteger/BigDecimal. BigInteger was later added in with the implementation of javascript BigInt.

So, I think Byte was overlooked at that time.

Long is probably the only other questionable one at this point.

[p-bakker]

Don't have any answers, but regarding adding another flag for 3: another option would be to clearly document what we do and don't map onto JavaScript primitives (and document why we skip some). If someone wants different behavior, creating a custom wrap factory is quite straight forward.

[p-bakker]

@gbrail @rbri Thoughts?