366725 - Avoid using JS_GetStringBytes and JS_GetStringChars internally

Status: RESOLVED FIXED

(Core :: JavaScript Engine, enhancement)

Description

[Igor Bukanov]

The functions JS_GetStringBytes and JS_GetStringChars are the only string functions that do not take JSContext* while accessing runtime structures/allocating memory. It would be nice to keep the functions only for compatibility while switching all the code to there JSContext* versions. See, for example, bug 366446, where the lack of JSContext* prevents to use some memory accounting schemas.

Comment 1

[Brendan Eich [:brendan]]

One brute force solution: #ifdef jsapi.h so embeddings that are ready for the new cx-parameterized APIs can just use the same old names, passing cx up front.

In any event, the internal calls should all be changed ASAP to js_* counterparts, which should be fixsed to take cx, not rt (as js_GetStringBytes does) or no such arg (as js_GetStringChars does).  Igor, how about an initial patch to clean these internal calls and functions up?

/be

Comment 2

[Igor Bukanov]

(In reply to comment #1)
> One brute force solution: #ifdef jsapi.h so embeddings that are ready for the
> new cx-parameterized APIs can just use the same old names, passing cx up front.

That does not work since the new API can return NULL. Thus the embeddings most likely will need to update their code in any case. Thus I suggest new names + warning printouts on the first call to the deprecated function. 

> 
> In any event, the internal calls should all be changed ASAP to js_*
> counterparts, which should be fixsed to take cx, not rt (as js_GetStringBytes
> does) or no such arg (as js_GetStringChars does).  Igor, how about an initial
> patch to clean these internal calls and functions up?

It does not work like that due to passing of nulls as JSContext. I am going to finish a patch soon that just duplicates the code for JS_GetString(Chars|Bytes) while removing null-checks for the function which I call js_StringTo(Chars|Bytes).

Comment 3

[Igor Bukanov]

I looked at the usage of JS_GetStringBytes. The majority of cases comes from various code fragments reporting errors, debugging printouts and decompiler. In all this cases calling JS_GetStringBytes is waste of memory and time to deflate string into a cache just to copy the deflated copy into the final destination.

Perhaps it is better to replace at least error reporting cases with explicit Deflate/JS_free calls?

Comment 4

[Brian Crowder]

Please make the new routines (if any) return const char * or const jschar *, instead of non-const?

Comment 5

[Igor Bukanov]

(In reply to comment #4)
> Please make the new routines (if any) return const char * or const jschar *,
> instead of non-const?

As far as I can see it is better to replace GetStringBytes for js_DeflateString/JSFree pair or similar functions using arena. In most cases where the function is used it is very unlikely that the byte cache will be reused.

Comment 6

[Brendan Eich [:brendan]]

Igor: errors are rare, I minimized error case code footprint by using JS_GetStringBytes (although some cases date back to when SpiderMonkey used 8-bit chars in JSStrings, most do not; I thought about getting fancier but decided it was not worth the code size for uncommon cases).  But if you can hack an even better way to report errors, please do.  E.g., with JS_C_STRINGS_ARE_UTF8, could or should we try to pass UTF-8 in C to the underlying reporter callback?

Brian's right, const-ipation is called for.  I still would hope to avoid keeping the bad old APIs undef some #ifdef, and the names are enviably clear, compared to JS_GetStringChars2 or JS_GetStringCharsForRealTakingContextFirstArg or whatever :-).

/be
/be

Comment 7

[Igor Bukanov]

(In reply to comment #6)
> Igor: errors are rare, I minimized error case code footprint by using
> JS_GetStringBytes (although some cases date back to when SpiderMonkey used
> 8-bit chars in JSStrings, most do not; I thought about getting fancier but
> decided it was not worth the code size for uncommon cases).  But if you can
> hack an even better way to report errors, please do.

We can define a version of ReportError that takes an extra argument defining the type of error message arguments, which can be char*, jschar* or JString *. 

Comment 8

[Igor Bukanov]

Attachment: Using proper API internally v0

This is a work in progress to use js_GetStringBytes(cx, str) and js_GetStringChars(cx, str) everywhere in SM. In few places the patch removes the calls altogether and in other it adds checks for a null result of the functions.

The patch does not yet change the return type of functions to const char*/const jschar*.

The patch assumes that the patch from bug 366809 is applied.

Comment 9

[Igor Bukanov]

Attachment: Using proper API internally v0.1

Compared with the previous version, the patch add const to the return type of js_GetStringBytes/js_GetStringChars.

The patch assumes that the patch from bug 366809 is applied.

Comment 11

[Igor Bukanov]

Attachment: Using proper API v1

The patch either changes code to call js_GetString(Chars|Bytes) with cx passed instead of JS_ versions or it replaces the calls by the code directly deflating the strings.

Comment 12

[Brian Crowder]

 JS_PUBLIC_API(char *)
 JS_GetStringBytes(JSString *str)
...
 JS_PUBLIC_API(jschar *)
 JS_GetStringChars(JSString *str)
...

Igor:  Did you experiment with making these return const char * and const jschar * respectively?

+     * with explicit malloc call and ignore its errors.
+     * If we fails to convert a dependent string into an independent one, our

Should be "If we fail to convert"...

I looked over the rest and didn't see anything obvious, but I'm not really qualified to review the bulk of this patch.  I was looking more out of curiosity.  Did you mean to r? brendan (currently no reviewer set)?

Comment 13

[Igor Bukanov]

(In reply to comment #12)
>  JS_PUBLIC_API(char *)
>  JS_GetStringBytes(JSString *str)
> ...
>  JS_PUBLIC_API(jschar *)
>  JS_GetStringChars(JSString *str)
> ...
> 
> Igor:  Did you experiment with making these return const char * and const
> jschar * respectively?

The patch does not touch the public API. Instead it switches SM internals to use js_GetString(Bytes|Chars) that returns const strings and take cx as argument. 

> I looked over the rest and didn't see anything obvious, but I'm not really
> qualified to review the bulk of this patch.  I was looking more out of
> curiosity.  Did you mean to r? brendan (currently no reviewer set)?

I will ask Brendan for a review.

Comment 14

[Igor Bukanov]

Attachment: Using proper API internally v1b

The is the previous patch with a comment fix.

Comment 15

[Brendan Eich [:brendan]]

Comment on attachment 252365 [details] [diff] [review]
Using proper API internally v1b

JS_GetStringChars
    Blank line in comment between first two (virtual) paragraphs.

date_toLocaleFormat
    Use JS_ConvertArguments for smaller code.

jsgc.c
    Are the null checks for js_GetStringBytes failures worth it?  *printf will format "(null)"
    which looks as good as or better than "(error)" to me, and saves sources and compiled (latter
    not an issue since only GC_MARK_DEBUG, but source size and complexity are issues) code size.

LogCall
    Please don't change this, it'll conflict with a patch I have and it's not important.

js_Interpret
    Tracing code elaboration to format "<error>" or "<null>" seems excessive to me -- tracing is
    not working well in all cases (timeless has details) and it's only if !JS_THREADED_INTERP.
    Even if fixed, it seems wrong to add so many lines when the output need not be perfect in the
    face of OOM errors. 

js_FindIdentifierBase
    The 'if (short || long_function_name(...,
                                         ...)) {'
    style is at odds with prevailing style -- break after || and don't indent quite so much.

SprintString
    Looks good!

Decompile
    Nice, js_puts of jp2's buffer directly!

FoldXMLConstants
    Same comments as above for tracing code and GC_MARK_DEBUG complexity -- my DEBUG_brendanXXX
    hacks do not care so much about errors that they want the extra source lines.

ReportCompileErrorNumber
    rs=me, I didn't extract and re-diff with -w (if you did and it looks good, great).

js_MarkScopeProperty
    Ditto overkill on tracing/debugging code size.

DumpSubtree
    Please don't change this, it's nonsense (note the cx param is not passed in!) and I have a fix
    that will conflict.

js_GetStringBytes
    s/calls this/calls us/ in the comment.

    Later, there's a gratuitous change to indentation that I think makes the code the tinyiest bit
    possible harder to read ;-), unindenting the JSSTRING_LENGTH(str) to underhang cx, instead of
    exactly underhanging JSSTRING_CHARS(str) -- I know, prevailing style underhangs first arg, but
    this was always an exception to that rule.

js_XDRCStringAtom
    Similar underhanging nit-picking applies here.

namespace_mark_vector, xml_mark_vector
    etc. on overdoing GC_MARK_DEBUG code.

Looks good otherwise.  Passes testsuite?

/be

Comment 16

[Igor Bukanov]

(In reply to comment #15)
>     Even if fixed, it seems wrong to add so many lines when the output need not
> be perfect in the
>     face of OOM errors. 

With JS_C_STRINGS_ARE_UTF8 defined the result of js_GetStringBytes will be null for any string that is not UTF16. This tells me that for debugging a version of uneval that does not print "" around the string would be a better option then js_GetStringBytes. 



> 
> js_FindIdentifierBase
>     The 'if (short || long_function_name(...,
>                                          ...)) {'
>     style is at odds with prevailing style -- break after || and don't indent
> quite so much.
> 
> SprintString
>     Looks good!
> 
> Decompile
>     Nice, js_puts of jp2's buffer directly!
> 
> FoldXMLConstants
>     Same comments as above for tracing code and GC_MARK_DEBUG complexity -- my
> DEBUG_brendanXXX
>     hacks do not care so much about errors that they want the extra source
> lines.
> 
> ReportCompileErrorNumber
>     rs=me, I didn't extract and re-diff with -w (if you did and it looks good,
> great).
> 
> js_MarkScopeProperty
>     Ditto overkill on tracing/debugging code size.
> 
> DumpSubtree
>     Please don't change this, it's nonsense (note the cx param is not passed
> in!) and I have a fix
>     that will conflict.
> 
> js_GetStringBytes
>     s/calls this/calls us/ in the comment.
> 
>     Later, there's a gratuitous change to indentation that I think makes the
> code the tinyiest bit
>     possible harder to read ;-), unindenting the JSSTRING_LENGTH(str) to
> underhang cx, instead of
>     exactly underhanging JSSTRING_CHARS(str) -- I know, prevailing style
> underhangs first arg, but
>     this was always an exception to that rule.
> 
> js_XDRCStringAtom
>     Similar underhanging nit-picking applies here.
> 
> namespace_mark_vector, xml_mark_vector
>     etc. on overdoing GC_MARK_DEBUG code.
> 
> Looks good otherwise.  Passes testsuite?
> 
> /be
> 

Comment 18

[Brendan Eich [:brendan]]

(In reply to comment #16)
> (In reply to comment #15)
> >     Even if fixed, it seems wrong to add so many lines when the output need not
> > be perfect in the
> >     face of OOM errors. 
> 
> With JS_C_STRINGS_ARE_UTF8 defined the result of js_GetStringBytes will be null
> for any string that is not UTF16. This tells me that for debugging a version of
> uneval that does not print "" around the string would be a better option then
> js_GetStringBytes. 

Hmm, that seems like a bug in js_GetStringBytes for JS_C_STRINGS_ARE_UTF8 -- but I am confused by what you meant by "not UTF16".  What JS strings cause js_GetStringBytes compiled with JS_C_STRINGS_ARE_UTF8 defined to return null?

/be

Comment 21

[Igor Bukanov]

(In reply to comment #18)
> Hmm, that seems like a bug in js_GetStringBytes for JS_C_STRINGS_ARE_UTF8 --
> but I am confused by what you meant by "not UTF16".  What JS strings cause
> js_GetStringBytes compiled with JS_C_STRINGS_ARE_UTF8 defined to return null?

Here is the code from js_DeflateStringToBuffer with JS_C_STRINGS_ARE_UTF8 defined:

        if ((c >= 0xDC00) && (c <= 0xDFFF))
            goto badSurrogate;

That is, js_DeflateStringToBuffer treats chars as UTF-16-encoded sequence of Unicode characters. The encoding uses the surrogate pairs and when the surrogate is invalid, the function returns null. 

To avoid that I will later attach a patch that adds:

extern size_t
js_PutEscapedStringImpl(char *buffer, size_t bufferSize, FILE *f,
                        JSString *str, uint32 quote);

/*
 * Write str into buffer escaping any non-printable or non-ASCII character.
 * Guarantees that a NUL is at the end of the buffer. Returns the length of
 * the written output, NOT including the NUL.
 * If buffer is null, just returns the length of the output.
 * If quote is not 0, it must be a single or double quote character that will
 * quote the output.  
*/
#define js_PutEscapedString(buffer, bufferSize, str, quote)                   \
    js_PutEscapedStringImpl(buffer, bufferSize, NULL, str, quote)

#define js_FileEscapedString(file, str, quote)                                \
    (JS_ASSERT(file), js_PutEscapedStringImpl(NULL, 0, file, str, quote))

The functions never fail and always escape non-ascii. IMO this is more useful for a debug output.

Comment 22

[Igor Bukanov]

Attachment: Implementation v2

Changes compared with the previous version:

1. To avoid GetStringBytes I added debug-only js_PutEscapedString and js_FileEscapedString that writes escaped strings to a string buffer/file. The function never fails and avoids introducing new GC-objects. Moreover, it reduced the source complexity of the tracing/debugging code compared with the current version.

2. I added 
extern size_t
js_GetDeflatedStringLength(JSContext *cx, const jschar *chars,
                           size_t charsLength);
since using js_DeflateStringToBuffer is very inconvenient when one wants just to calculate the required buffer size.

Comment 23

[Igor Bukanov]

Attachment: Implementation v2 (diff -b)

Comment 24

[Brendan Eich [:brendan]]

Comment on attachment 252768 [details] [diff] [review]
Implementation v2

Sorry, thought I reviewed this. Looks good, but the patch is out of date. Can you update a fresh one, and a diff -w one (no need to request review on that, I'll read it and stamp the full patch).

/be

Comment 25

[Igor Bukanov]

Attachment: Implementation v3

Here is a patch update to sync with CVS head.

Comment 26

[Igor Bukanov]

Attachment: Implementation v3 for real

I should write on the wall: check the content of the patch before attaching one.

Comment 27

[Igor Bukanov]

Attachment: Implementation v3 (diff -w)

Comment 28

[Brendan Eich [:brendan]]

(In reply to comment #27)
> Created an attachment (id=255339) [details]
> Implementation v3 (diff -w)

Set patch flag? Or is this really application/octet-stream?

/be

Comment 29

[Igor Bukanov]

Comment on attachment 255339 [details] [diff] [review]
Implementation v3 (diff -w)

This is a patch

Comment 30

[Brendan Eich [:brendan]]

Comment on attachment 255338 [details] [diff] [review]
Implementation v3 for real


>         /* let PRMJTime format it.       */
>-        result_len = PRMJ_FormatTime(buf, sizeof buf, format, &split);
>+        result_len = PRMJ_FormatTime(buf, sizeof buf, (char *)format, &split);

Sigh, we need a FIXME: citing a bug on prmjtime.c -- it needs constipation and probably other love.

> static void
> DumpSubtree(JSScopeProperty *sprop, int level, FILE *fp)
> {
>-    char buf[10];
>+    JSString *str;
>     JSScopeProperty *kids, *kid;
>     PropTreeKidsChunk *chunk;
>     uintN i;
> 
>-    fprintf(fp, "%*sid %s g/s %p/%p slot %lu attrs %x flags %x shortid %d\n",
>-            level, "",
>-            JSID_IS_ATOM(sprop->id)
>-            ? JS_GetStringBytes(ATOM_TO_STRING(JSID_TO_ATOM(sprop->id)))
>-            : JSID_IS_OBJECT(sprop->id)
>-            ? js_ValueToPrintableString(cx, OBJECT_JSID_TO_JSVAL(sprop->id))
>-            : (JS_snprintf(buf, sizeof buf, "%ld", JSVAL_TO_INT(sprop->id)),
>-               buf)
>+    fprintf(fp, "%*sid ", level, "");
>+    if (JSID_IS_ATOM(sprop->id)) {
>+        str =  ATOM_TO_STRING(JSID_TO_ATOM(sprop->id));
>+    } else if (JSID_IS_OBJECT(sprop->id)) {
>+        str = js_ValueToString(cx, OBJECT_JSID_TO_JSVAL(sprop->id));

Oops, still a broken dependency on cx here (note no such argument to DumpSubtree or its callers all the way back to js_GC -- could fix that two ways: add cx param from js_GC; remove cx dep here.

>+#if defined(DEBUG) ||                                                         \
>+    defined(GC_MARK_DEBUG) ||                                                 \
>+    defined(DUMP_CALL_TABLE) ||                                               \
>+    defined(DUMP_SCOPE_STATS)
>+size_t
>+js_PutEscapedStringImpl(char *buffer, size_t bufferSize, FILE *file,
>+                        JSString *str, uint32 quote)
>+{
>+    jschar *chars, *charsEnd;
>+    size_t n;
>+    char *charEscape;
>+    char c;
>+    uintN u, hex, shift;
>+    enum {
>+        STOP, FIRST_QUOTE, LAST_QUOTE, CHARS, ESCAPE_START, ESCAPE_MORE
>+    } state;
>+
>+    static char oneLetterEscapes[] =        "\b" "\f" "\n" "\r" "\t" "\v";
>+    static char oneLetterEscapesMapped[] =  "b"  "f"  "n"  "r"  "t"  "v";

Try to use js_EscapeMap (jsopcode.[ch]).

> /*
>  * Inflate bytes to JS chars into a buffer.
>  * 'chars' must be large enough for 'length' jschars.
>  * The buffer is NOT null-terminated.
>- * cx may be NULL, which means no errors are thrown.
>  * The destination length needs to be initialized with the buffer size, takes
>  * the number of chars moved.

May as well reformat this comment to wrap better while you are here.

>+/*
>  * Deflate JS chars to bytes into a buffer.
>  * 'bytes' must be large enough for 'length chars.
>  * The buffer is NOT null-terminated.
>- * cx may be NULL, which means no errors are thrown.
>  * The destination length needs to be initialized with the buffer size, takes
>  * the number of bytes moved.

Ditto.

>+/*
>+ * Write str into buffer escaping any non-printable or non-ASCII character.
>+ * Guarantees that a NUL is at the end of the buffer. Returns the length of
>+ * the written output, NOT including the NUL.
>+ * If buffer is null, just returns the length of the output.
>+ * If quote is not 0, it must be a single or double quote character that will
>+ * quote the output.

Or, if you prefer, put an extra blank comment line (spaces *\n) between paragraphs, here and below (and above in the mdaumli comments). Main thing I'm whining about is ragged right lines clumped together. Either the sentences should wrap into one paragraph, or if separate paras should be separated by "blank" line.

>+ *
>+ * The function is only defined for debug builds.
>+*/
>+#define js_PutEscapedString(buffer, bufferSize, str, quote)                   \
>+    js_PutEscapedStringImpl(buffer, bufferSize, NULL, str, quote)
>+
>+/*
>+ * Write str into file escaping any non-printable or non-ASCII character.
>+ * Returns the number of bytes written fto file.

"fto" typo and reformat per above.

>+ * If quote is not 0, it must be a single or double quote character that will
>+ * quote the output.
>+ *
>+ * The function is only defined for debug builds.
>+*/
>+#define js_FileEscapedString(file, str, quote)                                \
>+    (JS_ASSERT(file), js_PutEscapedStringImpl(NULL, 0, file, str, quote))
>+
> JS_END_EXTERN_C
> 
> #endif /* jsstr_h___ */

>     if (xdr->mode == JSXDR_ENCODE) {
>         JS_ASSERT(ATOM_IS_STRING(*atomp));
>-        bytes = JS_GetStringBytes(ATOM_TO_STRING(*atomp));
>-        return JS_XDRCString(xdr, &bytes);
>+        str = ATOM_TO_STRING(*atomp);
>+        bytes = js_DeflateString(xdr->cx,
>+                                 JSSTRING_CHARS(str),
>+                                 JSSTRING_LENGTH(str));
>+        if (!bytes)
>+            return JS_FALSE;
>+        ok = JS_XDRCString(xdr, &bytes);
>+        JS_free(xdr->cx, bytes);
>+        return ok;

Crazy: I forgot that the XDR code deflates strings. FIXME citing new bug, or is one on file already?

>+            n = (ns->prefix)
>+                ? js_PutEscapedString(ns->prefix, 0, buf, sizeof buf)
>+                : 0;

Nit: could avoid parenthesizing high-precedence ? condition expression.

/be

Comment 31

[Igor Bukanov]

(In reply to comment #30)
> (From update of attachment 255338 [details] [diff] [review])
> 
> >         /* let PRMJTime format it.       */
> >-        result_len = PRMJ_FormatTime(buf, sizeof buf, format, &split);
> >+        result_len = PRMJ_FormatTime(buf, sizeof buf, (char *)format, &split);
> 
> Sigh, we need a FIXME: citing a bug on prmjtime.c -- it needs constipation and
> probably other love.

Since PR_FormatTime in NSPR already got const, http://lxr.mozilla.org/seamonkey/source/nsprpub/pr/src/misc/prtime.c#1675 ,I will add it to PRMJ_FormatTime within this patch.

Comment 32

[Igor Bukanov]

Attachment: Implementation v4

This version fixes the nits (accept FIXME for js_XDRCStringAtom as it already got one at the function start), makes sure that SM compiles with -DDUMP_SCOPE_STATS -DGC_MARK_DEBUG -DDUMP_CALL_TABLE (previously I used the wrong order of arguments  in few places when calling js_PutEscapedString) and fixes GC_MARK_DEBUG crashes when dumping XML: now the patch do not assume that prefix is non null in Namespace/QName.

Comment 33

[Igor Bukanov]

Attachment: Implementation v4 (diff -w)

Comment 34

[Brendan Eich [:brendan]]

Comment on attachment 255483 [details] [diff] [review]
Implementation v4

+const char js_EscapeMap[2][10] = {
+    { '\b', '\f', '\n', '\r', '\t', '\v', '"', '\'', '\\', '\0' },
+    { 'b',  'f',  'n',  'r',  't',  'v',  '"', '\'', '\\', '\0' }
 };

Does this really improve code size, runtime, or readability? I'm thinking not.

         /* Use js_EscapeMap, \u, or \x only if necessary. */
-        if ((u = js_strchr(js_EscapeMap, c)) != NULL) {
+        if (!(c >> 8) && (e = strchr(js_EscapeMap[0], (int)c)) != NULL) {

Most input chars will make the first test true, at least in this hemisphere.

             ok = dontEscape
                  ? Sprint(sp, "%c", (char)c) >= 0
-                 : Sprint(sp, "\\%c", (char)u[1]) >= 0;
+                 : Sprint(sp, "\\%c",
+                          js_EscapeMap[1][e - js_EscapeMap[0]]) >= 0;

u[1] is also faster and less code than the new version.

@@ -4006,16 +4006,12 @@
     if (argc == 0) {
         str = cx->regExpStatics.input;
         if (!str) {
-            const char *bytes = js_GetStringBytes(cx, re->source);
-
-            if (bytes) {
-                JS_ReportErrorNumber(cx, js_GetErrorMessage, NULL,
-                                     JSMSG_NO_INPUT,
-                                     bytes,
-                                     (re->flags & JSREG_GLOB) ? "g" : "",
-                                     (re->flags & JSREG_FOLD) ? "i" : "",
-                                     (re->flags & JSREG_MULTILINE) ? "m" : "");
-            }
+            JS_ReportErrorNumber(cx, js_GetErrorMessage, NULL,
+                                 JSMSG_NO_INPUT,
+                                 JS_GetStringBytes(re->source),
+                                 (re->flags & JSREG_GLOB) ? "g" : "",
+                                 (re->flags & JSREG_FOLD) ? "i" : "",
+                                 (re->flags & JSREG_MULTILINE) ? "m" : "");
             ok = JS_FALSE;
             goto out;
         }

This hunk goes back to JS_GetStringBytes, saving code for an error case but possibly using "" on OOM for the regexp source in the error message.

@@ -4032,12 +4028,16 @@
     if (argc == 0) {
         str = cx->regExpStatics.input;
         if (!str) {
-            JS_ReportErrorNumber(cx, js_GetErrorMessage, NULL,
-                                 JSMSG_NO_INPUT,
-                                 JS_GetStringBytes(re->source),
-                                 (re->flags & JSREG_GLOB) ? "g" : "",
-                                 (re->flags & JSREG_FOLD) ? "i" : "",
-                                 (re->flags & JSREG_MULTILINE) ? "m" : "");
+            const char *bytes = js_GetStringBytes(cx, re->source);
+
+            if (bytes) {
+                JS_ReportErrorNumber(cx, js_GetErrorMessage, NULL,
+                                     JSMSG_NO_INPUT,
+                                     bytes,
+                                     (re->flags & JSREG_GLOB) ? "g" : "",
+                                     (re->flags & JSREG_FOLD) ? "i" : "",
+                                     (re->flags & JSREG_MULTILINE) ? "m" : "");
+            }
             ok = JS_FALSE;
             goto out;
         }

This goes the other way. Why the difference?

+ * Inflate bytes to JS chars into a buffer. 'chars' must be large enough for
+ * 'length' jschars. The buffer is NOT null-terminated. The destination length
+ * needs to be initialized with the buffer size, takes the number of chars
+ * moved.

Need "and" or something like it to like the first clause in the last sentence with the "takes the number of chars moved."

+ * Deflate JS chars to bytes into a buffer. 'bytes' must be large enough for
+ * 'length chars. The buffer is NOT null-terminated. The destination length
+ * needs to be initialized with the buffer size, takes the number of bytes
+ * moved.

Ditto.

+extern size_t
+js_PutEscapedStringImpl(char *buffer, size_t bufferSize, FILE *f,

Nit: canonical name is fp, not f.

/be

Comment 35

[Igor Bukanov]

Attachment: Implementation v5

Addressing the nits. The new version still changes js_EscapeMap, but this time it just converts it from jschar to char so a compiler-optimized strchr can be used for the search.

Comment 36

[Igor Bukanov]

Attachment: Implementation v5 (diff -w)

Comment 37

[Brendan Eich [:brendan]]

Comment on attachment 257239 [details] [diff] [review]
Implementation v5

Sorry, I forgot to stamp this!

/be

Comment 38

[Igor Bukanov]

Changing the title to reflect the nature of the patch: it does not deprecate JS_GetString(Bytes|Chars). Rather it makes sure that the functions are not used internally. Deprecating would be done in a separated patch. 

Comment 39

[Igor Bukanov]

I committed the patch from comment 35 to the trunk:

Checking in jsapi.c;
/cvsroot/mozilla/js/src/jsapi.c,v  <--  jsapi.c
new revision: 3.309; previous revision: 3.308
done
Checking in jsdate.c;
/cvsroot/mozilla/js/src/jsdate.c,v  <--  jsdate.c
new revision: 3.84; previous revision: 3.83
done
Checking in jsexn.c;
/cvsroot/mozilla/js/src/jsexn.c,v  <--  jsexn.c
new revision: 3.82; previous revision: 3.81
done
Checking in jsfun.c;
/cvsroot/mozilla/js/src/jsfun.c,v  <--  jsfun.c
new revision: 3.177; previous revision: 3.176
done
Checking in jsgc.c;
/cvsroot/mozilla/js/src/jsgc.c,v  <--  jsgc.c
new revision: 3.205; previous revision: 3.204
done
Checking in jsinterp.c;
/cvsroot/mozilla/js/src/jsinterp.c,v  <--  jsinterp.c
new revision: 3.338; previous revision: 3.337
done
Checking in jsnum.c;
/cvsroot/mozilla/js/src/jsnum.c,v  <--  jsnum.c
new revision: 3.78; previous revision: 3.77
done
Checking in jsobj.c;
/cvsroot/mozilla/js/src/jsobj.c,v  <--  jsobj.c
new revision: 3.332; previous revision: 3.331
done
Checking in jsopcode.c;
/cvsroot/mozilla/js/src/jsopcode.c,v  <--  jsopcode.c
new revision: 3.212; previous revision: 3.211
done
Checking in jsopcode.h;
/cvsroot/mozilla/js/src/jsopcode.h,v  <--  jsopcode.h
new revision: 3.43; previous revision: 3.42
done
Checking in jsparse.c;
/cvsroot/mozilla/js/src/jsparse.c,v  <--  jsparse.c
new revision: 3.270; previous revision: 3.269
done
Checking in jsregexp.c;
/cvsroot/mozilla/js/src/jsregexp.c,v  <--  jsregexp.c
new revision: 3.135; previous revision: 3.134
done
Checking in jsscan.c;
/cvsroot/mozilla/js/src/jsscan.c,v  <--  jsscan.c
new revision: 3.121; previous revision: 3.120
done
Checking in jsscope.c;
/cvsroot/mozilla/js/src/jsscope.c,v  <--  jsscope.c
new revision: 3.58; previous revision: 3.57
done
Checking in jsscope.h;
/cvsroot/mozilla/js/src/jsscope.h,v  <--  jsscope.h
new revision: 3.43; previous revision: 3.42
done
Checking in jsscript.c;
/cvsroot/mozilla/js/src/jsscript.c,v  <--  jsscript.c
new revision: 3.138; previous revision: 3.137
done
Checking in jsstr.c;
/cvsroot/mozilla/js/src/jsstr.c,v  <--  jsstr.c
new revision: 3.139; previous revision: 3.138
done
Checking in jsstr.h;
/cvsroot/mozilla/js/src/jsstr.h,v  <--  jsstr.h
new revision: 3.35; previous revision: 3.34
done
Checking in jsxdrapi.c;
/cvsroot/mozilla/js/src/jsxdrapi.c,v  <--  jsxdrapi.c
new revision: 1.31; previous revision: 1.30
done
Checking in jsxml.c;
/cvsroot/mozilla/js/src/jsxml.c,v  <--  jsxml.c
new revision: 3.146; previous revision: 3.145
done
Checking in prmjtime.c;
/cvsroot/mozilla/js/src/prmjtime.c,v  <--  prmjtime.c
new revision: 3.59; previous revision: 3.58
done
Checking in prmjtime.h;
/cvsroot/mozilla/js/src/prmjtime.h,v  <--  prmjtime.h
new revision: 3.16; previous revision: 3.15
done

Comment 40

[Alex Vincent [:WeirdAl]]

c:/mozilla.org/trunk/mozilla/js/src/jsinterp.c(6052) : error C2143: syntax error : missing ';' before 'else'

Comment 41

[Robert Sayre]

(In reply to comment #40)
> c:/mozilla.org/trunk/mozilla/js/src/jsinterp.c(6052) : error C2143: syntax
> error : missing ';' before 'else'
> 

~/firefox/mozilla/js/src> cvs commit -m "fix for bug 366725 missed a semicolon and broke msvc 2005 express for some people." jsinterp.c
Checking in jsinterp.c;
/cvsroot/mozilla/js/src/jsinterp.c,v  <--  jsinterp.c
new revision: 3.339; previous revision: 3.338
done

Comment 42

[Igor Bukanov]

(In reply to comment #41)
> (In reply to comment #40)
> > c:/mozilla.org/trunk/mozilla/js/src/jsinterp.c(6052) : error C2143: syntax
> > error : missing ';' before 'else'
> > 
> 
> ~/firefox/mozilla/js/src> cvs commit -m "fix for bug 366725 missed a semicolon
> and broke msvc 2005 express for some people." jsinterp.c
> Checking in jsinterp.c;
> /cvsroot/mozilla/js/src/jsinterp.c,v  <--  jsinterp.c
> new revision: 3.339; previous revision: 3.338
> done

Thanks! 

Comment 43

[Igor Bukanov]

Attachment: Implementation v5 + build fix

This the committed patch + build fix for references. I forgot to test the patch with -DJS_THREADED_INTERP=0.

Comment 44

[Bob Clary [:bc] (inactive)]

With JS_C_STRINGS_ARE_UTF8 defined, js shell fails to build

jsstr.c:2841: error: too few arguments to function ‘js_GetDeflatedStringLength’

Comment 45

[Igor Bukanov]

(In reply to comment #44)
> With JS_C_STRINGS_ARE_UTF8 defined, js shell fails to build
> 
> jsstr.c:2841: error: too few arguments to function
> ‘js_GetDeflatedStringLength’
> 

I will fix this later today.

Comment 46

[Igor Bukanov]

Attachment: Fixing JS_C_STRINGS_ARE_UTF8 issues

The patch makes things compile and fixes couple of warnings due char versus unsigned char madness. I just wish signed chars would never exist...

Comment 47

[Brendan Eich [:brendan]]

Comment on attachment 259933 [details] [diff] [review]
Fixing JS_C_STRINGS_ARE_UTF8 issues

r=me, sorry for the tardy review.

/be

Comment 48

[Igor Bukanov]

i committed the patch from comment 46 to the trunk to fix JS_C_STRINGS_ARE_UTF8 compilation:

Checking in jsprf.c;
/cvsroot/mozilla/js/src/jsprf.c,v  <--  jsprf.c
new revision: 3.23; previous revision: 3.22
done
Checking in jsstr.c;
/cvsroot/mozilla/js/src/jsstr.c,v  <--  jsstr.c
new revision: 3.140; previous revision: 3.139
done