Closed
Bug 366725
Opened 18 years ago
Closed 18 years ago
Avoid using JS_GetStringBytes and JS_GetStringChars internally
Categories
(Core :: JavaScript Engine, enhancement)
Core
JavaScript Engine
Tracking
()
RESOLVED
FIXED
People
(Reporter: igor, Assigned: igor)
References
Details
Attachments
(4 files, 11 obsolete files)
(deleted),
patch
|
brendan
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
Details | Diff | Splinter Review | |
(deleted),
patch
|
igor
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
brendan
:
review+
|
Details | Diff | Splinter Review |
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•18 years ago
|
||
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
Assignee | ||
Comment 2•18 years ago
|
||
(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).
Assignee | ||
Comment 3•18 years ago
|
||
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?
Depends on: 366809
Comment 4•18 years ago
|
||
Please make the new routines (if any) return const char * or const jschar *, instead of non-const?
Assignee | ||
Comment 5•18 years ago
|
||
(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•18 years ago
|
||
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
Assignee | ||
Comment 7•18 years ago
|
||
(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 *.
Status: NEW → ASSIGNED
Assignee | ||
Comment 8•18 years ago
|
||
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.
Assignee | ||
Comment 9•18 years ago
|
||
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.
Assignee | ||
Comment 10•18 years ago
|
||
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.
Attachment #252317 -
Flags: review?
Comment 11•18 years ago
|
||
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)?
Assignee | ||
Comment 12•18 years ago
|
||
(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.
Assignee | ||
Comment 13•18 years ago
|
||
The is the previous patch with a comment fix.
Attachment #252365 -
Flags: review?(brendan)
Updated•18 years ago
|
Attachment #252317 -
Flags: review?
Comment 14•18 years ago
|
||
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
Assignee | ||
Comment 15•18 years ago
|
||
(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 16•18 years ago
|
||
(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
Assignee | ||
Comment 17•18 years ago
|
||
(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.
Assignee | ||
Comment 18•18 years ago
|
||
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.
Attachment #252768 -
Flags: review?(brendan)
Assignee | ||
Comment 19•18 years ago
|
||
Comment 20•18 years ago
|
||
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
Attachment #252768 -
Flags: review?(brendan)
Updated•18 years ago
|
Attachment #252365 -
Flags: review?(brendan)
Assignee | ||
Comment 21•18 years ago
|
||
Here is a patch update to sync with CVS head.
Attachment #251473 -
Attachment is obsolete: true
Attachment #251512 -
Attachment is obsolete: true
Attachment #252317 -
Attachment is obsolete: true
Attachment #252365 -
Attachment is obsolete: true
Attachment #252768 -
Attachment is obsolete: true
Attachment #252770 -
Attachment is obsolete: true
Attachment #255337 -
Flags: review?(brendan)
Assignee | ||
Comment 22•18 years ago
|
||
I should write on the wall: check the content of the patch before attaching one.
Attachment #255337 -
Attachment is obsolete: true
Attachment #255338 -
Flags: review?(brendan)
Attachment #255337 -
Flags: review?(brendan)
Assignee | ||
Comment 23•18 years ago
|
||
Comment 24•18 years ago
|
||
(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
Assignee | ||
Comment 25•18 years ago
|
||
Comment on attachment 255339 [details] [diff] [review]
Implementation v3 (diff -w)
This is a patch
Attachment #255339 -
Attachment is patch: true
Attachment #255339 -
Attachment mime type: application/octet-stream → text/plain
Comment 26•18 years ago
|
||
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
Attachment #255338 -
Flags: review?(brendan) → review-
Assignee | ||
Comment 27•18 years ago
|
||
(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.
Assignee | ||
Comment 28•18 years ago
|
||
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.
Attachment #255338 -
Attachment is obsolete: true
Attachment #255339 -
Attachment is obsolete: true
Attachment #255483 -
Flags: review?(brendan)
Assignee | ||
Comment 29•18 years ago
|
||
Comment 30•18 years ago
|
||
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
Assignee | ||
Comment 31•18 years ago
|
||
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.
Attachment #255483 -
Attachment is obsolete: true
Attachment #255484 -
Attachment is obsolete: true
Attachment #257239 -
Flags: review?(brendan)
Attachment #255483 -
Flags: review?(brendan)
Assignee | ||
Comment 32•18 years ago
|
||
Comment 33•18 years ago
|
||
Comment on attachment 257239 [details] [diff] [review]
Implementation v5
Sorry, I forgot to stamp this!
/be
Attachment #257239 -
Flags: review?(brendan) → review+
Assignee | ||
Comment 34•18 years ago
|
||
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.
Assignee | ||
Updated•18 years ago
|
Summary: Deprecate JS_GetStringBytes and JS_GetStringChars → Avoid using JS_GetStringBytes and JS_GetStringChars internally
Assignee | ||
Comment 35•18 years ago
|
||
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
Status: ASSIGNED → RESOLVED
Closed: 18 years ago
Resolution: --- → FIXED
Comment 36•18 years ago
|
||
c:/mozilla.org/trunk/mozilla/js/src/jsinterp.c(6052) : error C2143: syntax error : missing ';' before 'else'
Comment 37•18 years ago
|
||
(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
Assignee | ||
Comment 38•18 years ago
|
||
(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!
Assignee | ||
Comment 39•18 years ago
|
||
This the committed patch + build fix for references. I forgot to test the patch with -DJS_THREADED_INTERP=0.
Attachment #257799 -
Flags: review+
Updated•18 years ago
|
Flags: in-testsuite-
Comment 40•18 years ago
|
||
With JS_C_STRINGS_ARE_UTF8 defined, js shell fails to build
jsstr.c:2841: error: too few arguments to function ‘js_GetDeflatedStringLength’
Assignee | ||
Comment 41•18 years ago
|
||
(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.
Assignee | ||
Comment 42•18 years ago
|
||
The patch makes things compile and fixes couple of warnings due char versus unsigned char madness. I just wish signed chars would never exist...
Attachment #259933 -
Flags: review?(brendan)
Comment 43•18 years ago
|
||
Comment on attachment 259933 [details] [diff] [review]
Fixing JS_C_STRINGS_ARE_UTF8 issues
r=me, sorry for the tardy review.
/be
Attachment #259933 -
Flags: review?(brendan) → review+
Assignee | ||
Comment 44•18 years ago
|
||
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
You need to log in
before you can comment on or make changes to this bug.
Description
•