Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Resolved JsCreateString & JsPointerToString #6669 #6739

Closed
wants to merge 1 commit into from

Conversation

RahulSharma-10
Copy link

@RahulSharma-10 RahulSharma-10 commented Aug 30, 2021

Since I am new to Open-Source, I picked this issue. Following the instructions, I have:

  1. Changed the SAL annotation for the string input parameter from In to In_opt, I also read up relevant documentation for the same which helped in understanding NULL Value handling.

  2. I have also changed the implementation, added the test cases & updated the comments in the headers which mention that 0 lengths will ignore the input string pointer and return a Js empty string.

  3. The Added test cases that were previously not working are working now.

  4. I also glanced through the active contributions for the same issue by @dagarxji, and tried to solve the last issue raised by @rhuanjl regarding the PERFORM_JSRT_TTD_RECORD_ACTION macro & checked in case it is inside an if-conditional.

  5. I spent a majority of my time understanding Chakra & the code-flow, the thought behind has been really instructional. Already loving the inclusivity of the community.

@rhuanjl
Copy link
Collaborator

rhuanjl commented Aug 31, 2021

Thanks for having a go at this.

Having a look through this looks incomplete at the moment.

There are 3 APIs you've looked at, for each one we need:

  • header SAL annotation change
  • header comments update
  • implementation (JSRT.cpp) SAL annotation update
  • implementation logic update - to handle the stringLength=0 case
  • test update

Taking them in turn, it currently looks like you've done:

API header SAL header comments implementation SAL implementation logic test
JsCreateString yes yes yes yes
JsCreateStringUTF16 yes
JsPointerToString yes yes yes yes yes

Please let me know if you need any further help.

@RahulSharma-10
Copy link
Author

Hi @rhuanjl, Thank you for the prompt response. Will work to rectify the issues pointed out now!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants