superreview granted: [Bug 224337] Mozilla crashes viewing hectorplasmic.com : [Attachment 137343] a patch

Brendan Eich <brendan@mozilla.org> has granted Jungshik Shin
<jshin@mailaps.org>'s request for superreview:

Bug 224337: Mozilla crashes viewing hectorplasmic.com
http://bugzilla.mozilla.org/show_bug.cgi?id=224337

Attachment 137343: a patch
http://bugzilla.mozilla.org/attachment.cgi?id=137343&action=edit

------- Additional Comments from Brendan Eich <brendan@mozilla.org>

>+#define DEFINE_CCMAP(var, type)	      \
>+type static union {			      \
>+  PRUint16 array[var ## SIZE];	      \
>+  ALU_TYPE align;			      \
>+} var ## Union =			      \
>+{					      \
>+  { var ## INITIALIZER }		      \
>+};					      \
>+type static PRUint16* var = var ## Union.array;
>+
>+#define DEFINE_X_CCMAP(var, type)			  \
>+type	static union {					  \
>+  PRUint16 array[var ## SIZE];			  \
>+  ALU_TYPE align;					  \
>+} var ## Union =					  \
>+{							  \
>+  { var ## INITIALIZER }				  \
>+};							  \
>+type	static PRUint16* var = var ## Union.array +  CCMAP_EXTRA; 

I always factor macros, so it seems better to me to have DEFINE_CCMAP take
(varname, typequal, extra) as params: varname is var above, typequal is type
(better name, const is not a type, it's a type qualifier in C/C++), and extra
is 0 or CCMAP_EXTRA.  If you do that, then you don't need more macros, but if
you want conveniences that eliminate the extra parameter, you could call the
base macro DEFINE_ANY_CCMAP and call it from DEFINE_CCMAP and DEFINE_X_CCMAP
one-liners.

_SIZE and _INITIALIZER seem like better suffixes, althought it really doesn't
matter. The resulting macro name will have interCapsALLCAPS name style the way
you do it, and interCaps_ALLCAPS with this suggestions.  Matter of taste.

>+DEFINE_CCMAP(gDoubleByteSpecialCharsCCMap, )

Check the C standard.  This should be ok, but if not, just pass /* nothing */
instead of an empty argument.  May want to do that to be safe.

> # When CCMap is accessed, (PRUint16 *) is casted to 

Nit: "cast to", not "casted to".

Looks great otherwise, thanks for fixing this!

/be
0
bugzilla
12/13/2003 7:45:38 PM
netscape.mozilla.reviewers 29156 articles. 0 followers. Follow

0 Replies
289 Views

Similar Articles

[PageSpeed] 10

Reply: