Add API functions to create string from a valid UTF-8 string. #1430

robertsipka · 2016-11-14T10:06:13Z

I think this patch needs to be followed by another, which contains symbol (functions names, types, ...) renames. Because CESU-8 is a variant of UTF-8, the names was left utf8 and mentioned the CESU-8 only in comments after #616, but I think this is very confusing right now. For example the 'ecma_new_ecma_string_from_utf8' function check that the string is a cesu-8 encoded string at the first step, so it doesn't accept an utf-8 encoded string which contains 4 bytes long characters, and throw an assert.

zherczeg · 2016-11-14T12:15:24Z

jerry-core/jerry.c

+
+  dest_buf_size = lit_calculate_cesu8_size_from_utf8 ((lit_utf8_byte_t *) str_p, (lit_utf8_size_t) str_size);
+
+  if ((lit_utf8_size_t) str_size != dest_buf_size)


I think this only handles ASCII strings. We should also allow strings which does not have 4 byte characters.

It will handle all utf8 encoded string.

zherczeg · 2016-11-14T12:17:21Z

jerry-core/lit/lit-strings.c

+  while (size < buf_size)
+  {
+    if ((buf_p[size] & LIT_UTF8_4_BYTE_MASK) == LIT_UTF8_4_BYTE_MARKER)
+    {


Somehow we should return with this information.

zherczeg · 2016-11-14T12:18:37Z

jerry-core/jerry.c

+
+  if ((lit_utf8_size_t) str_size != dest_buf_size)
+  {
+    lit_utf8_byte_t dest_buf[dest_buf_size];


I would not allocate a big buffer on stack. We should pre-allocate a string for this, copy the string there, and set the string flags.

Indeed, but I think we should use JMEM_DEFINE_LOCAL_ARRAY also and it wouldn't require too much modification, what is your opinion?

@zherczeg : I modified this part, and I defined a local array variable and allocated the memory for the array on the heap, please check it.

zherczeg · 2016-11-15T09:47:46Z

jerry-core/jerry.c

+
+  if ((lit_utf8_size_t) str_size != dest_buf_size)
+  {
+    JMEM_DEFINE_LOCAL_ARRAY (dest_buf, dest_buf_size, lit_utf8_byte_t);


I still don't think we should allocate memory here. We could allocate memory for the final string directly. It is a bit more complicated, but I think memory reduction is critcal.

robertsipka · 2016-11-15T13:34:52Z

@zherczeg : Thanks for the advice, I've modified this patch based on your comments.

zherczeg · 2016-11-15T13:39:41Z

jerry-core/lit/lit-strings.h

@@ -135,4 +135,6 @@ ecma_char_t lit_utf8_peek_prev (const lit_utf8_byte_t *);
 void lit_utf8_incr (const lit_utf8_byte_t **);
 void lit_utf8_decr (const lit_utf8_byte_t **);

+bool lit_utf8_contains_four_bytes_unicode (const lit_utf8_byte_t *, lit_utf8_size_t);


I liked the original approach that the size is also computed by this function, and it can be passed to ecma_new_ecma_string_from_utf8_converted_to_cesu8 to avoid double scan.

Yes, it was a double scan, so I had an idea based on your suggestion and I made a small change on this checking parts. I hope you like this way too.

zherczeg · 2016-11-15T13:41:06Z

jerry-core/ecma/base/ecma-helpers-string.c

@@ -216,6 +216,111 @@ ecma_new_ecma_string_from_utf8 (const lit_utf8_byte_t *string_p, /**< utf-8 stri
 } /* ecma_new_ecma_string_from_utf8 */

 /**
+ * Allocate new ecma-string and fill it with characters from the utf8 string
+ * It converts all 4-bytes long unicode sequence to two 3-bytes long sequence


Allocate a new ecma-string and initialize it from the utf8 string argument.
All 4-bytes long unicode sequences are converted into two 3-bytes long sequences.

zherczeg · 2016-11-15T13:42:48Z

The patch is definitely going in the right direction. Some finishing touch is still needed.

zherczeg · 2016-11-15T14:09:06Z

jerry-core/ecma/base/ecma-helpers-string.c

+    str_length++;
+  }
+
+  JERRY_ASSERT (size == string_size);


If this is true, there is no need to computing it except for debugging purposes. This would simplify the for loop above.

I've renamed the misleading variable names.

LaszloLango

LGTM

zherczeg · 2016-11-15T14:59:08Z

I think the patch is ok. @LaszloLango do you think we should update the documentation and some api tests in this or a follow-up patch?

robertsipka · 2016-11-16T07:13:46Z

Thanks, I've rebased it with master. @LaszloLango : documentation and some api tests in a follow-up patch is ok for you too?

JerryScript-DCO-1.0-Signed-off-by: Robert Sipka rsipka.uszeged@partner.samsung.com

LaszloLango · 2016-11-16T07:49:05Z

Yes, follow-up patch is fine. Just don't forget to do it. :)

zherczeg · 2016-11-16T09:32:22Z

LGTM
Please also reference this patch for the appropriate issue.

robertsipka force-pushed the api_utf8 branch 2 times, most recently from 1c51485 to 8210285 Compare November 14, 2016 10:39

zherczeg reviewed Nov 14, 2016

View reviewed changes

robertsipka force-pushed the api_utf8 branch from 8210285 to c02585b Compare November 15, 2016 08:21

zherczeg reviewed Nov 15, 2016

View reviewed changes

robertsipka force-pushed the api_utf8 branch from c02585b to a7a9e4e Compare November 15, 2016 13:34

zherczeg reviewed Nov 15, 2016

View reviewed changes

robertsipka force-pushed the api_utf8 branch 2 times, most recently from 717f31a to 6a3dec5 Compare November 15, 2016 14:07

LaszloLango added api Related to the public API feature request Requested feature labels Nov 15, 2016

zherczeg reviewed Nov 15, 2016

View reviewed changes

robertsipka force-pushed the api_utf8 branch from 6a3dec5 to c851e1a Compare November 15, 2016 14:32

LaszloLango approved these changes Nov 15, 2016

View reviewed changes

robertsipka force-pushed the api_utf8 branch from c851e1a to cc1c4c8 Compare November 16, 2016 07:12

Add API functions to create string from a valid UTF-8 string.

6f9353a

JerryScript-DCO-1.0-Signed-off-by: Robert Sipka rsipka.uszeged@partner.samsung.com

robertsipka force-pushed the api_utf8 branch from cc1c4c8 to 6f9353a Compare November 16, 2016 07:35

robertsipka merged commit ffaca58 into jerryscript-project:master Nov 16, 2016

robertsipka mentioned this pull request Nov 16, 2016

Rename symbols from ...utf8... to ...cesu8... #1268

Closed

robertsipka deleted the api_utf8 branch November 24, 2016 08:48


		dest_buf_size = lit_calculate_cesu8_size_from_utf8 ((lit_utf8_byte_t *) str_p, (lit_utf8_size_t) str_size);

		if ((lit_utf8_size_t) str_size != dest_buf_size)

Add API functions to create string from a valid UTF-8 string. #1430

Add API functions to create string from a valid UTF-8 string. #1430

Uh oh!

Conversation

robertsipka commented Nov 14, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

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

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

robertsipka commented Nov 15, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

zherczeg commented Nov 15, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

LaszloLango left a comment

Choose a reason for hiding this comment

Uh oh!

zherczeg commented Nov 15, 2016

Uh oh!

robertsipka commented Nov 16, 2016

Uh oh!

LaszloLango commented Nov 16, 2016

Uh oh!

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

Uh oh!

Uh oh!

Uh oh!

robertsipka Nov 15, 2016 •

edited

Loading

zherczeg commented Nov 16, 2016 •

edited

Loading