657537 - Make CESU8 boolean into a behaviory enum

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Chris Leary [:cdleary] (not checking bugmail)]

Readability fix requested by Waldo in bug 654301.

Comment 1

[Chris Leary [:cdleary] (not checking bugmail)]

Attachment: 0. Add CESU8 behavior parameter.

Comment 2

[Chris Leary [:cdleary] (not checking bugmail)]

Attachment: 1. Namespace and comment in/de-flation helpers.

Comment 3

[Chris Leary [:cdleary] (not checking bugmail)]

Attachment: 1. Namespace and comment in/de-flation helpers.

Whoops, was missing some browser files.

Comment 4

[Jeff Walden [:Waldo]]

Comment on attachment 533069 [details] [diff] [review]
0. Add CESU8 behavior parameter.

Review of attachment 533069 [details] [diff] [review]:
-----------------------------------------------------------------

"FlationCoding" is a weird name; it seems to me "Encoding" would be a better name, if perhaps only marginally better.  I guess at that point the name of the corresponding variable has to pick up the slack.  I'd suggest |srcEncoding|, |inflatedEncoding|, etc. for variable/parameter names to help.

Also, NormalEncoding doesn't mean anything.  It should be named more precisely, e.g. ISO88591Encoding or Latin1Encoding.  I'd prefer the former, but I could take either.

::: js/src/jsstr.h
@@ +1115,4 @@
>   * CESU-8 (Compatibility Encoding Scheme for UTF-16: 8-bit) is a
>   * variant of UTF-8 that allows us to store any wide character
>   * string as a narrow character string. For strings containing
>   * mostly ascii, it saves space.

Rewrap this paragraph to wrap at 80/79 characters.

Comment 5

[Jeff Walden [:Waldo]]

Comment on attachment 533130 [details] [diff] [review]
1. Namespace and comment in/de-flation helpers.

Review of attachment 533130 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/src/jscntxt.cpp
@@ +1018,5 @@
>                  JS_ASSERT(expandedArgs == argCount);
>                  *out = 0;
>                  cx->free_(buffer);
> +                *messagep = DeflateString(cx, reportp->ucmessage,
> +                                          (size_t)(out - reportp->ucmessage));

Make that a size_t() C++-style cast.

::: js/src/jsstr.cpp
@@ +4074,5 @@
>      return NULL;
>  }
>  
>  jschar *
> +js::InflateString(JSContext *cx, const char *bytes, size_t *lengthp, FlationCoding fc)

I think generally we dump functions inside namespace js blocks when defining them, rather than use a js:: prefix to indicate that we're defining the namespace-nested function.

@@ +4112,5 @@
>  /*
>   * May be called with null cx.
>   */
>  char *
> +js::DeflateString(JSContext *cx, const jschar *chars, size_t nchars)

Ditto.

@@ +4138,5 @@
>      return bytes;
>  }
>  
>  size_t
> +js::GetDeflatedStringLength(JSContext *cx, const jschar *chars, size_t nchars)

Ditto.  (And at some point you get enough that the block makes more sense even on its own merits.)

@@ +4151,5 @@
>   * May be called with null cx through public API, see below.
>   */
>  size_t
> +js::GetDeflatedUTF8StringLength(JSContext *cx, const jschar *chars,
> +                                size_t nchars, FlationCoding fc)

La la la

@@ +4197,5 @@
>  }
>  
> +bool
> +js::DeflateStringToBuffer(JSContext *cx, const jschar *src, size_t srclen,
> +                          char *dst, size_t *dstlenp)

La

@@ +4223,5 @@
>  }
>  
> +bool
> +js::DeflateStringToUTF8Buffer(JSContext *cx, const jschar *src, size_t srclen,
> +                              char *dst, size_t *dstlenp, FlationCoding fc)

La

@@ +4287,5 @@
>  }
>  
> +bool
> +js::InflateStringToBuffer(JSContext *cx, const char *src, size_t srclen,
> +                          jschar *dst, size_t *dstlenp)

La!

@@ +4314,5 @@
>  }
>  
> +bool
> +js::InflateUTF8StringToBuffer(JSContext *cx, const char *src, size_t srclen,
> +                              jschar *dst, size_t *dstlenp, FlationCoding fc)

LA!

::: js/src/jsstr.h
@@ +1116,5 @@
> + * - Some string functions have an optional FlationCoding argument that allow
> + *   the caller to force CESU-8 encoding handling. 
> + * - Functions that don't take a FlationCoding base their NormalEncoding
> + *   behavior on the js_CStringsAreUTF8 value. NormalEncoding is either raw
> + *   (simple zero-extension) or UTF-8 depending on js_CStringsAreUTF8.

...wait.  I hate this interface.

I guess take back what I said about ISO88591Encoding in the previous review.

Comment 6

[Chris Leary [:cdleary] (not checking bugmail)]

http://hg.mozilla.org/tracemonkey/rev/2781c17e531d

Comment 7

[Chris Leary [:cdleary] (not checking bugmail)]

http://hg.mozilla.org/mozilla-central/rev/2781c17e531d