Fixes Jsrt API C99/C++98 compatability #2001

jcoffland · 2016-11-14T00:14:37Z

ChakraCore.h appears to desire compatibility with C99 however this is currently broken due to the use of nullptr, bool and C++11 style enums. Fortunately these problems are easily remedied.

msftclas · 2016-11-14T00:14:40Z

Hi @jcoffland, I'm your friendly neighborhood Microsoft Pull Request Bot (You can call me MSBOT). Thanks for your contribution!

In order for us to evaluate and accept your PR, we ask that you sign a contribution license agreement. It's all electronic and will take just minutes. I promise there's no faxing. https://cla.microsoft.com.

TTYL, MSBOT;

msftclas · 2016-11-14T00:34:35Z

@jcoffland, Thanks for signing the contribution license agreement so quickly! Actual humans will now validate the agreement and then evaluate the PR.

Thanks, MSBOT;

obastemur · 2016-11-15T08:09:43Z

@jcoffland could you please squash the commits ? That should also trigger CI

jianchun · 2016-11-15T16:46:42Z

lib/Jsrt/ChakraCommon.h

@@ -71,6 +71,10 @@ typedef BYTE* ChakraBytePtr;
 #define CHAKRA_API extern "C" SET_API_VISIBILITY JsErrorCode
 #else
 #define CHAKRA_API extern     SET_API_VISIBILITY JsErrorCode
+#include <stdbool.h>
+#endif
+#if !defined(__cplusplus) || (__cplusplus <= 199711L)


nit pick: Could you please insert a blank line before this #if for readability?

jcoffland · 2016-11-15T20:26:29Z

Ok, I've added the whitespace and squashed the commits.

jcoffland · 2016-11-28T23:11:53Z

Any chance this will get merged? It's a very basic fix and I followed all the guidelines. Will someone please take responsibility for merging this PR?

jianchun · 2016-11-28T23:22:48Z

@jcoffland CI has been failing on this PR weirdly. Could you please try squash (still seeing 3 commits) and rebase onto latest master and force push your PR? Thanks!

jcoffland · 2016-11-29T01:22:41Z

Ok, I believe I've sorted it out. I had already squashed my commits but then updated my fork by doing a merge which added extra commits to this PR. I fixed this by redoing the fork update as a rebase.

obastemur · 2016-11-29T11:38:05Z

@dotnet-bot test this please

jianchun · 2016-11-29T17:22:57Z

Did a search and found

JsTTDMoveMode moveMode = (JsTTDMoveMode)(JsTTDMoveMode::JsTTDMoveKthEvent | ((int64) startEventCount) << 32);

Looks current JsTTDMoveMode needs 64 bits.

@mrkmarron

jcoffland · 2016-11-29T21:23:55Z

@jianchun good catch. I found the line you quoted above https://github.com/Microsoft/ChakraCore/blob/master/bin/ch/ch.cpp#L275. I did a little investigating and found that JsTTDMoveMode was hijacked to also pass in an event count. You can see it being used at https://github.com/Microsoft/ChakraCore/blob/master/lib/Jsrt/Jsrt.cpp#L3781. It appears that JsTTDGetSnapTimeTopLevelEventMove() is the only place JsTTDMoveMode is used in this way and https://github.com/Microsoft/ChakraCore/blob/master/bin/ch/ch.cpp#L275 is the only place it's ever called like this. In fact, this is the only place JsTTDGetSnapTimeTopLevelEventMove() is called.

Abusing JsTTDMoveMode like this seems like a bad idea since it is very likely to cause confusion. It's not at all obvious from looking at the definition of JsTTDMoveMode that it is going to be used in this way. The easiest solution is to add a kthEvent parameter to JsTTDGetSnapTimeTopLevelEventMove(). I've implemented this change.

mrkmarron · 2016-11-29T21:54:24Z

Hi @jcoffland, you are 100% correct that I was doing some parameter hijacking. I agree with you that this is suboptimal and converting the API to use a second parameter instead is more appropriate (thanks for your help cleaning this up).

One comment I have is that kthEvent is an optional parameter so we should probably make that clear in the SAL annotation/name.

@jianchun we will need to update the use of this API in the node/xplat branch when we merge later as well.

jcoffland · 2016-11-29T22:05:55Z

@mrkmarron right. I've now changed the parameter annotation from _In_ to _In_opt.

dilijev · 2016-11-29T22:27:43Z

Question for ChakraCore maintainers:

Is it possible to add a sort of test suite which exercises the use of our API when embedding ChakraCore in C99 and C++98 projects (using legacy build compatibility modes in vc and/or clang)?

On that note, @liminzhu, I remember someone mentioning that one or more samples had broken when we updated the API. Should we think about including a set of tests that include the samples to ensure we don't unexpectedly break anything? (I guess we could clone the repo https://github.com/Microsoft/Chakra-Samples as a step in the tests, or include it as a submodule of ChakraCore for this purpose.)

dilijev · 2016-11-29T22:30:57Z

On another note:

Should this fix be back-ported to release/1.4, release/1.3, and/or release/1.2 because we still support 1.2 and 1.3, and we had intended this to work?

jcoffland · 2016-11-29T22:35:40Z

It wasn't an issue in 1.3 when I tried it.

dilijev · 2016-11-29T23:01:43Z

@jcoffland Thanks for confirming that. In any case, I think this PR should probably be re-targeted to 1.4 (because I'm pretty sure this issue would affect that release). But, hold off on that until we confirm. Worst-case we can just merge to master and then back-port the change to 1.4.

obastemur · 2016-12-01T02:12:45Z

I remember someone mentioning that one or more samples had broken when we updated the API. Should we think about including a set of tests that include the samples to ensure we don't unexpectedly break anything?

@dilijev We already test that for some time. See https://github.com/Microsoft/ChakraCore/tree/master/test/native-tests

jcoffland · 2016-12-01T05:35:19Z

@obastemur Do those tests check that the samples in https://github.com/Microsoft/Chakra-Samples work?

obastemur · 2016-12-01T14:56:04Z

@jcoffland AFAIK we do check a bit more here. However, we should add more tests there indeed.

jcoffland · 2016-12-01T19:46:58Z

It would make sense to trigger CI builds on https://github.com/Microsoft/Chakra-Samples to make sure they build. It would be best to have CI build badges on that project's README.md. The reason being that the samples are one of the first places new users start with ChakraCore. If it doesn't work they are likely to go elsewhere.

obastemur · 2016-12-01T22:54:14Z

It would make sense to trigger CI builds on https://github.com/Microsoft/Chakra-Samples to make sure they build.

I am not sure if I was being clear. We already build and test the Jsrt usage (with the same xplat compile scripts and same code) on ChakraCore CI for each PR.

If CI here is not failing, it is unlikely that we break Chakra-Samples. That is what https://github.com/Microsoft/ChakraCore/tree/master/test/native-tests for.

jcoffland · 2016-12-01T23:54:13Z

@obastemur What keeps native-tests in sync with Chakra-Samples? My experience when I tried building Chakra-Samples was that they would not compile against ChakraCore master. This makes for a disheartening introduction.

Anyway, I've gone off topic. Can this PR be merged now?

obastemur · 2016-12-02T01:06:35Z

What keeps native-tests in sync with Chakra-Samples?

We do. Anything is failing on native tests, should alert devs to update Chakra Samples. IMHO it is similar to updating documentation.

obastemur · 2016-12-02T01:28:13Z

Can this PR be merged now?

Sorry for keeping this PR open for longer than it should. @dilijev any update?

In the mean time #2136 to make sure we don't break it again.

Fixed abuse of JsTTDMoveMode enum in call to JsTTDGetSnapTimeTopLevelEventMove() so that JsTTDMoveMode does not need to be 64-bit and thus incompatible with C99/C++98 Don't use nullptr at all, it's incompatible with C++98

dilijev · 2016-12-05T18:41:24Z

This issue will affect release/1.4 so we should probably backport from master to release/1.4 it at a minimum.

Other than that, LGTM and good to merge.

dilijev · 2016-12-07T03:11:21Z

should alert devs to update Chakra Samples

@obastemur while this seems true in principle, I think it is easy to miss, and since, unlike documentation (which, except for JSRT APIs where we look closely at public documentation, is basically best-effort), Samples are something which we expect to work and which we can actually break. People new to the project would expect it to work right out of the box without having to work through any issues.

I think we should seriously think about adding some automated test mechanism to ensure they all continue to work.

/cc @liminzhu

dilijev · 2016-12-07T03:53:52Z

Assigning myself. I'll test and merge.

Merge pull request #2001 from CauldronDevelopmentLLC:master

jcoffland · 2016-12-08T23:11:21Z

It should be noted that there are additional compile problems using the Jsrt API with the MSVC compiler in C mode that are not addressed by this PR.

Thanks for the merge!

dilijev · 2016-12-09T00:27:28Z

@jcoffland Thanks for the heads-up. Could you open an issue so we can track those issues?

@dilijev

…ak again Merge pull request #2136 from obastemur:add_c99_test #2001 fixes Jsrt C99/C++98 compatibility issue. @dilijev brought the idea of adding a test to make sure we don't break it again. This native test should take care of that. p.s.: This PR will break until #2001 is merged. /cc @dilijev @jcoffland

msftclas added the cla-required label Nov 14, 2016

msftclas added cla-signed and removed cla-required labels Nov 14, 2016

jianchun reviewed Nov 15, 2016

View reviewed changes

jcoffland force-pushed the master branch from efb1ca2 to 4645b61 Compare November 15, 2016 20:25

jcoffland force-pushed the master branch 2 times, most recently from 8a87583 to 348bf15 Compare November 29, 2016 01:15

jcoffland force-pushed the master branch from df36d92 to 11263d9 Compare November 29, 2016 21:21

jcoffland changed the title ~~Fixes Jsrt API C99 compatability~~ Fixes Jsrt API C99/C++98 compatability Nov 29, 2016

jcoffland force-pushed the master branch from 11263d9 to aa4dc6a Compare November 29, 2016 22:04

jcoffland force-pushed the master branch from aa4dc6a to 1ea8931 Compare November 29, 2016 22:09

dilijev added the Compatibility label Nov 29, 2016

obastemur mentioned this pull request Dec 2, 2016

Add C++98 native test to ensure we don't break again #2136

Merged

Fixes Jsrt API C99/C++98 compatability

979a9f3

Fixed abuse of JsTTDMoveMode enum in call to JsTTDGetSnapTimeTopLevelEventMove() so that JsTTDMoveMode does not need to be 64-bit and thus incompatible with C99/C++98 Don't use nullptr at all, it's incompatible with C++98

jcoffland force-pushed the master branch from 1ea8931 to 979a9f3 Compare December 4, 2016 11:03

dilijev mentioned this pull request Dec 7, 2016

Add tests (here or in Chakra-Samples) to ensure ChakraCore Samples don't break. #2160

Open

dilijev self-assigned this Dec 7, 2016

dilijev mentioned this pull request Dec 7, 2016

Update usage of JsTTDGetSnapTimeTopLevelEventMove in xplat branch #2161

Closed

chakrabot merged commit 979a9f3 into chakra-core:master Dec 7, 2016

chakrabot pushed a commit that referenced this pull request Dec 7, 2016

[MERGE #2001 @jcoffland] Fixes Jsrt API C99/C++98 compatability

986201a

Merge pull request #2001 from CauldronDevelopmentLLC:master

Fixes Jsrt API C99/C++98 compatability #2001

Fixes Jsrt API C99/C++98 compatability #2001

Uh oh!

Conversation

jcoffland commented Nov 14, 2016

Uh oh!

msftclas commented Nov 14, 2016

Uh oh!

msftclas commented Nov 14, 2016

Uh oh!

obastemur commented Nov 15, 2016

Uh oh!

jianchun Nov 15, 2016

Choose a reason for hiding this comment

Uh oh!

jcoffland commented Nov 15, 2016

Uh oh!

jcoffland commented Nov 28, 2016

Uh oh!

jianchun commented Nov 28, 2016

Uh oh!

jcoffland commented Nov 29, 2016

Uh oh!

obastemur commented Nov 29, 2016

Uh oh!

jianchun commented Nov 29, 2016

Uh oh!

jcoffland commented Nov 29, 2016

Uh oh!

mrkmarron commented Nov 29, 2016

Uh oh!

jcoffland commented Nov 29, 2016

Uh oh!

dilijev commented Nov 29, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dilijev commented Nov 29, 2016

Uh oh!

jcoffland commented Nov 29, 2016

Uh oh!

dilijev commented Nov 29, 2016

Uh oh!

obastemur commented Dec 1, 2016

Uh oh!

jcoffland commented Dec 1, 2016

Uh oh!

obastemur commented Dec 1, 2016

Uh oh!

jcoffland commented Dec 1, 2016

Uh oh!

obastemur commented Dec 1, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jcoffland commented Dec 1, 2016

Uh oh!

obastemur commented Dec 2, 2016

Uh oh!

obastemur commented Dec 2, 2016

Uh oh!

dilijev commented Dec 5, 2016

Uh oh!

dilijev commented Dec 7, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dilijev commented Dec 7, 2016

Uh oh!

jcoffland commented Dec 8, 2016

Uh oh!

dilijev commented Dec 9, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

dilijev commented Nov 29, 2016 •

edited

Loading

obastemur commented Dec 1, 2016 •

edited

Loading

dilijev commented Dec 7, 2016 •

edited

Loading

dilijev commented Dec 9, 2016 •

edited

Loading