Fix for JsPointerToString and JsCreateString #6669 #6737
Conversation
Update for JsPointerToString and JsCreateString to ignore input string if length is 0. The function will return an empty string instead of JsErrorNullArgument.
There was a problem hiding this comment.
Thanks for the PR, always good to see a new contributor.
I've added a couple of comments below, but as well as that:
- please could you sign our contribution agreement (add name and username to bottom of this file): https://github.com/chakra-core/ChakraCore/blob/master/ContributionAgreement.md
- please update the copyright banners at the top of the files you've edited, see top of this file for example updated copyright: https://github.com/chakra-core/ChakraCore/blob/master/lib/Runtime/LibraryFunction.h
bin/NativeTests/JsRTApiTest.cpp
Outdated
| // Test JsPointerToString and JsCreateString on NULL inputs | ||
| JsValueRef nullStr; | ||
| REQUIRE(JsPointerToString(NULL, 0, &nullStr) == JsNoError); | ||
|
|
||
| JsValueRef nullFname; | ||
| REQUIRE(JsCreateString(NULL, 0, &nullFname) == JsNoError); | ||
|
|
There was a problem hiding this comment.
Please could you use spaces instead of tabs for the indent (here and other files) - our code base uses spaces consistently.
lib/Jsrt/Core/JsrtCore.cpp
Outdated
| if (length == 0) | ||
| { | ||
| content = ""; | ||
| } |
There was a problem hiding this comment.
Please could you bring this in line with the way you've done JsPointerToString below? If length = 0, skip the PARAM_NOT_NULL(content); and then use scriptContext->GetLibrary()->GetEmptyString(); instead of the logic on lines 896 and 898 below.
There was a problem hiding this comment.
Oh, I see. I had issues with GetEmptyString() because I was including line 898. Executing 896 and 898 only when length isn't zero fixed this. Thank you.
Update for JsPointerToString and JsCreateString to ignore input string if length is 0. The function will return an empty string instead of JsErrorNullArgument.
… into chakracore.git
|
Well noted. I have updated the files as requested. Hopefully that will remove all of the issues. |
|
Looks good apart from the test failure. I think it may be because you've put the PERFORM_JSRT_TTD_RECORD_ACTION macro inside the 'if' in one of the functions - can you move that so it's always called then hopefully the test will pass. |
Moved PERFORM_JSRT_TTD_RECORD_ACTION to execute outside of if statement.
| Js::JavascriptString *stringValue = Js::LiteralStringWithPropertyStringPtr:: | ||
| NewFromCString(content, (CharCount)length, scriptContext->GetLibrary()); | ||
|
|
||
| NewFromCString(content, (CharCount)length, scriptContext->GetLibrary()); |
There was a problem hiding this comment.
These calls should not happen on the length = 0 path, may be the cause of the test failures
#6669
Tagged as per issue request:
@rhuanjl