microsoft / ChakraCore-wiki

A mirror of the ChakraCore wiki to enable pull requests on the Wiki.
https://github.com/Microsoft/ChakraCore/wiki
Other
22 stars 30 forks source link

JsCreateString(Utf16) requires active script context? #41

Closed xiaoyinl closed 7 years ago

xiaoyinl commented 7 years ago

It seems to me that JsCreateString and JsCreateStringUtf16 require an active script context. They both return JsErrorNoCurrentContext if JsSetCurrentContext is not called earlier. But the doc doesn't mention they "require an active script context" in the "Remarks" section. I wonder if this note should be added.

The following is the test code:

#include <iostream>
#include <ChakraCore.h>
#pragma comment(lib, "ChakraCore.lib")

int main(void)
{
    char s[] = "hello";
    wchar_t str[] = L"hello";
    JsValueRef val;
    JsErrorCode err;
    if ((err = JsCreateStringUtf16((const uint16_t*)str, sizeof(str), &val)) != JsNoError)
    {
        std::wcout << L"JsCreateStringUtf16 fails with error " << err << L"\n";
    }
    if ((err = JsCreateString(s, sizeof(s), &val)) != JsNoError)
    {
        std::wcout << L"JsCreateString fails with error " << err << L"\n";
    }
    getchar();
    return 0;
}
msftclas commented 7 years ago

@xiaoyinl, Thanks for having already signed the Contribution License Agreement. Your agreement was validated by Microsoft. We will now review your pull request. Thanks, Microsoft Pull Request Bot

dilijev commented 7 years ago

@liminzhu Does this require updating the doc comments in the source code as well?

liminzhu commented 7 years ago

@dilijev it's a good idea to change the remark in header too.

xiaoyinl commented 7 years ago

@liminzhu I just realized I actually updated JsCopyStringUtf16 (instead of JsCreateStringUtf16) and JsCreateString. Do JsCopyString and JsCopyStringUtf16 also require active script context? If so, I will update this patch to update all four APIs.

liminzhu commented 7 years ago

No copystring actually does not require script context.

xiaoyinl commented 7 years ago

@dilijev @liminzhu I updated the remarks in the header too.

xiaoyinl commented 7 years ago

I force pushed a new commit. Now it only updates two JsCreateString APIs.