Add JSRT API JsLessThan (Fix #3568)

[xiaoyinl]

This PR implements the API JsLessThan and JsLessThanOrEqual, proposed in issue #3568.

Fixes #3568

Todo:

• Add more test cases
• Update documents (Add reference for JsLessThan and JsLessThanOrEqual microsoft/ChakraCore-wiki#43)

[xiaoyinl]

@dotnet-bot test Ubuntu shared_ubuntu_linux_debug please

[dilijev]

Would it make sense to add the other operators as well? (LessThanOrEqual, GreaterThan, GreaterThanOrEqual) I know that they can all be implemented in terms of JsLessThan and JsEqual, but adding wrappers for these comparison primitives to help user code read better doesn't seem like a bad idea. @liminzhu @Cellule opinions?

[dilijev]

@xiaoyinl When you make your next update to add documentation (assuming you still have a work item there), could you rebase this PR to a single commit?

[xiaoyinl]

@dilijev The way this PR implements LessThan is that, as proposed by @liminzhu, it takes a parameter allowsEqual to control whether this API is LessThan or LessThanOrEqual. I also think maybe having two separate functions JsLessThan and JsLessThanOrEqual makes code more readable than JsLessThan(..., true, ...).

If you have decided to separate JsLessThan and JsLessThanOrEqual, I'll update this PR, and rebase it at that time.

[dilijev]

@xiaoyinl I didn't realize the discussion on the issue at first. I'm not familiar with the discussion here. In general we want to keep the API surface small so let's not add a bunch more operations without more unanimous approval. But I agree with you that JsLessThan(..., true, ...) doesn't do a good job of capturing the idea of <=.

IMO having that functionality might be regarded as worse than only allowing JsLessThan(...) || JsEqual(...) as a way of doing this (especially since we have an idea of Equal (==) and StrictEqual (===)), because it's so opaque to the reader.

/cc @liminzhu @bterlson @marktjsonar who were involved in the issue discussion.

[xiaoyinl]

@dilijev If I remember it correctly, there is an edge case that makes JsLessThan(...) || JsEqual(...) not equivalent to JsLessThanOrEqual(...):

let a = undefined;
a <= a;  // false
a < a;   // false
a == a;  // true
a === a; // true

[liminzhu]

Ole! Thanks for the PR @xiaoyinl !

@xiaoyinl @dilijev having separate JsLessThan and JsLessThanOrEqual sounds fine to me. I'd hold off on > and >=. The only time someone would want it with the presence of < and <= is when the sequence of evaluating lhs and rhs matters, which does not sound common to me. It's probably better if we wait till a legitimate scenario presents itself before we add extra APIs.

JsLessThan || JsEquals is not a good proxy for <=. Besides what @xiaoyinl mentioned, JS is all messed up when comparing objects (not saying someone'd intentionally do this, but that's another foot gun). Example,

let x = {a: 1};
let y = {b: 2};
x > y // false
x < y // false
x == y // false
x === y // false
x >= y // true
x <= y // true

Edit: forgot to mention. @xiaoyinl can you put the API in ChakraCore.h instead? ChakraCore.h APIs only ship with ChakraCore. ChakraCommon.h APIs get shipped with Chakra on Windows as well, and there's a process we need to follow internally before that can happen.

[dilijev]

LGTM, but I'll let others review and approve.

[liminzhu]

@akroshg who is a good person to code-review JSRT changes?

[dilijev]

@liminzhu I would have thought you. :) Maybe @boingoing?

[liminzhu]

@dilijev well I care much about the interface but usually don't look into .cpp's so another pair of eyes would be nice :)

[boingoing]

I'm pretty sure we will need a new TTD action for recording these. Right now we're recording doing a non-strict-equals with RecordJsRTEquals and the flag value false to indicate not is-strict. We probably want to add a new action like RecordJsRTLessThan with a flag to say if it should be or-equal.

[boingoing]

Code changes look good to me but we should add a TTD action.

[boingoing]

Looks like this should pass true for allowsEqual, right?

[boingoing]

Sorry I should have mentioned earlier. We need an execute function to match the TTD action we recorded. Then when we replay we will know which action to execute.

This is simple, though, just needs a little glue code.

First we'll need to add the handler. Take a look at EqualsAction_Execute in TTActionEvents.cpp to get an idea of what to do. This is mostly just boiler-plate - execute the less-than or less-than-equal based on the flag you saved earlier.

Second we'll need to do the mapping between the action and the execute function. Check out TTEventLog.cpp near line 553. I think you'll want to add something like this:

TTD_CREATE_EVENTLIST_VTABLE_ENTRY_COMMON(LessThanActionTag, ContextAPIWrapper, JsRTDoubleVarSingleScalarArgumentAction, LessThanAction_Execute);

[xiaoyinl]

@boingoing Thank you for the detailed explanation. I have done what you mentioned. Hopefully this time I didn't miss anything.

[boingoing]

This looks good to me, thank you for the attention to detail. 👍

[xiaoyinl]

Thank you again for the review! I have squashed the commits.

[dilijev]

LGTM

[dilijev]

Testing internally; then will merge.