336575 - Remove mShiftTable from all encoders and decoders that don't use it

Status: RESOLVED FIXED

(Core :: Internationalization, defect)

Description

[Simon Montagu :smontagu]

Comment 1

[Simon Montagu :smontagu]

Attachment: patch

Comment 2

[Simon Montagu :smontagu]

By the way, I didn't version nsIUnicodeDecodeHelper because I'm about to file a bug to deCOMtaminate it: it's only ever used inside uconv and I don't see any reason for it to be an XPCOM interface.

Comment 3

[Simon Montagu :smontagu]

Comment on attachment 220761 [details] [diff] [review]
patch

I need to merge this to bug 356609 anyway, and I think I can do better than this. Right now all table-based encoder/decoders have a shiftTable, and most of them are empty. This makes nsUCSupport and the helpers more compact, but at the cost of adding 12 unused bytes to dozens of individual encoder/decoders. That seems like the wrong trade-off to me.

Comment 4

[Simon Montagu :smontagu]

It turns out that out of 87 decoders and 108 encoders, only 3 encoders and 1 decoder actually use the shiftTable.

Comment 5

[Simon Montagu :smontagu]

Attachment: patch

Comment 6

[Simon Montagu :smontagu]

Comment on attachment 222857 [details] [diff] [review]
patch

>Index: intl/uconv/util/nsUCConstructors.cpp
>+CreateMultiTableEncoder(PRInt32 aTableCount,
>+                        uScanClassID * aScanClassArray,
>+                        uMappingTable ** aMappingTable,
>+                        PRUint32 aMaxLengthFactor,
>+                        nsISupports* aOuter,
>+                        REFNSIID aIID,
>+                        void** aResult)
>+{
>+  return CreateMultiTableEncoder(aTableCount, aScanClassArray,
>+                                 aMappingTable, aMaxLengthFactor,
>+                                 aOuter, aIID, aResult);
>+}
>+

Oops, this should be 

+  return CreateMultiTableEncoder(aTableCount, aScanClassArray,
+                                 nsnull,
+                                 aMappingTable, aMaxLengthFactor,
+                                 aOuter, aIID, aResult);

I won't attach a new patch for this one line

Comment 7

[Jungshik Shin]

Comment on attachment 222857 [details] [diff] [review]
patch

r=jshin
Sorry for the very long delay.

Will you 'sanitize' indentation in unicpriv.h and take cvs blame? 

>-PRBool 	uGenerate(  uShiftTable *shift,
>+PRBool 	uGenerate(  uScanClassID scanClass,
>                     PRInt32* state, 
....
>-PRBool 	uScan(      uShiftTable *shift, 
>+PRBool 	uScan(      uScanClassID scanClass,
>+                    PRInt32 *state, 
>+                    unsigned char *in,
.....

>+PRBool 	uGenerateShift(  uShiftOutTable *shift,
>+                    PRInt32* state, 
....
>+PRBool 	uScanShift( uShiftInTable *shift, 
>                     PRInt32 *state, 
>                     unsigned char *in,
....


Perhaps, it's better to realign the argument list here?

>+MODULE_PRIVATE PRBool uScanShift(  
>+                            uShiftInTable    *shift,
>+                            PRInt32*    state,
>+                            unsigned char  *in,
>+                            PRUint16    *out,
>+                            PRUint32     inbuflen,
>+                            PRUint32*    inscanlen
>+                            );

BTW, while you're touching uscan.c, can you fix a typo ( 'gennerator' has
double n's)? There are two instances. Both are not changed by the patch, but
they're close to lines modified.

http://lxr.mozilla.org/seamonkey/search?string=uSubGennerator

Comment 8

[Simon Montagu :smontagu]

Comment on attachment 222857 [details] [diff] [review]
patch

Thanks for review. I'll pick those nits before checking in.

Comment 9

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 222857 [details] [diff] [review]
patch

rs=roc

Comment 10

[Simon Montagu :smontagu]

Checked in.