104158 - use NS_LITERAL_STRING instead of AssignWithConversion/EqualsWithConversion where possible

Status: RESOLVED INCOMPLETE

(Core :: XPCOM, defect, P1)

Description

[Boris Zbarsky [:bzbarsky]]

Filing this because Alec says he's already got it in his tree

Comment 1

[Alec Flett]

Attachment: editor conversion

Comment 2

[Alec Flett]

ack, I had the whole tree, but I ran my script once more and it whacked half my
files.. more tomorrow.

Comment 3

[Alec Flett]

I can't get the platform testing on this done by 0.9.6

Comment 4

[jag (Peter Annema)]

Alec, when do you think you'll get to this?

Comment 5

[Alec Flett]

its pretty low on my radar right now :( I welcome anyone else to take this
over... I just have too many other bugs to deal with..

Comment 6

[jag (Peter Annema)]

-> me

Comment 7

[jag (Peter Annema)]

Attachment: Replace a bunch of WithConversion(" with NS_LITERAL_STRING

Comment 8

[Alec Flett]

Comment on attachment 61012 [details] [diff] [review]
Replace a bunch of WithConversion(" with NS_LITERAL_STRING

rs=alecf, assuming you've built on all 3 platforms. 

If you haven't built on all 3 platforms, then sr=alecf on the code you have
built (i.e. land it asap) and then sr=alecf on the code as you test it..

The only reason I bring this up is that I made a run at this before, and found
that there were a few goofy uses of AssignWithConversion that required me to
hand-tweak a line or two. If that code is in Mac/Win and you've only tested
Linux, you might bust Mac or Win.

Anyway... this is great!

Comment 9

[jag (Peter Annema)]

Did that, I now have Windows building, and as soon as I'm done wrestling my Mac 
I'll attach an updated patch.

Comment 10

[jag (Peter Annema)]

Comment on attachment 61012 [details] [diff] [review]
Replace a bunch of WithConversion(" with NS_LITERAL_STRING

And I have r=bryner

Comment 11

[jag (Peter Annema)]

Pushing back

Comment 12

[jag (Peter Annema)]

Attachment: Patch that was checked in.

Comment 13

[Alec Flett]

I really hate to say this, but according to the tinderbox logs, it looks like
this slowed down pageloading time..:(

Comment 14

[jag (Peter Annema)]

Yes it did, by approx. 10ms on Linux, since it has to use NS_ConvertASCIItoUCS2 
there. On Mac and Windows it won't have, since they use nsDependentString.

Comment 15

[jag (Peter Annema)]

Checked in the third patch, I'll keep this bug open for now for any stragglers I
run across between now and getting rid of *WithConversion.

Comment 16

[Roy Yokoyama]

jag:  I am getting error message with today's pull

UnicharSelfTest.cpp
E:\raptor\mozilla\intl\unicharutil\tests\UnicharSelfTest.cpp(391) : error C2002
 invalid wide-character constant
E:\raptor\mozilla\intl\unicharutil\tests\UnicharSelfTest.cpp(391) : error C2002
 invalid wide-character constant
E:\raptor\mozilla\intl\unicharutil\tests\UnicharSelfTest.cpp(438) : error C2002
 invalid wide-character constant
E:\raptor\mozilla\intl\unicharutil\tests\UnicharSelfTest.cpp(438) : error C2002
 invalid wide-character constant
NMAKE : fatal error U1077: 'C:\MICROS~1\VC98\BIN\cl.exe' : return code '0x2'
Stop.
NMAKE : fatal error U1077: '"C:\MICROSOFT VISUAL STUDIO\VC98\BIN\NMAKE.exe"' :
eturn code '0x2'
Stop.

I am not certain why MS compiler doesn't like
inString.Assign(NS_LITERAL_STRING("\x61\x62\x80\xA0\x63")); with Japanese 
system locale setting.  I changed my DOS command shell to 'us' to use the
en locale; but it didn't help.  I think MSVC++ is doing something wacky.

I may be the only NS developer (occationally) compiling moz under Win98-Japanese;
however, we need to get this resolved for Japanese Moz contributors. :)
Note, this error *may* occur for other OS languages as well.

Comment 17

[Alec Flett]

what version of msvc? have you installed any VC++ service packs? you might try
upgrading to VC++ service pack 5...

Comment 18

[jag (Peter Annema)]

If that still doesn't work, replace those two lines with this:

437   nsString inString(NS_ConvertASCIItoUCS2("\x61\x62\x80\xA0\x63"));

I don't see why L"\x61\x62\x80\xA0\x63" (one of the steps in NS_LITERAL_STRING
on Windows) wouldn't work, though.

Comment 19

[jag (Peter Annema)]

See bug 115786.

Comment 20

[Roy Yokoyama]

cc myself
I'm not sure if applying the service pack fixes the problem.
(waiting from momoi-san's upgrade :)
However, while I was compiling Moz with -UNICODE flag,
http://bugzilla.mozilla.org/attachment.cgi?id=60387&action=view
I had the similar problem with  L"\x61\x62\x80\xA0\x63" 

I was forced to use TEXT("\x61\x62\x80\xA0\x63");
to work around the compilation error.

Comment 21

[Roy Yokoyama]

jag: I havent' tried TEXT() with Win98-JA build yet......

Comment 22

[jag (Peter Annema)]

Roy, what does TEXT() do? Will it work on all platforms?

Comment 23

[Roy Yokoyama]

>Will it work on all platforms?
No. I think it's only for Win32. :(

After reading the online help on TEXT() and the header file, it may not help us
after all. :(
Do you think we can fool the compiler by doing the 2)?  
I don't think so; but I am not very much of a compiler guru.

1) MS Online Help:
  The TEXT macro identifies a string as Unicode when the UNICODE is defined
during compilation.     
  Otherwise, it identifies a string as an ANSI string. 

  TEXT(
    LPTSTR string  // ANSI or Unicode string
  );
  Parameters
    string Pointer to the string to be interpreted as either Unicode or ANSI. 

2) Header file
  #ifdef UNICODE
    #define __TEXT(quote) L##quote
  #else
    #define __TEXT(quote) quote
  #endif
  #define TEXT(quote) __TEXT(quote)

Comment 24

[jag (Peter Annema)]

Hmmm... That is kinda what NS_LITERAL_STRING does internally, see
nsLiteralString.h, except it's based on whether wchar_t is unsigned 16 bits.

This is what NS_LITERAL_STRING("\x61\x62\x80\xA0\x63") expands to:

NS_STATIC_CAST(const nsAFlatString&,
               nsDependentString(NS_REINTERPRET_CAST(const PRUnichar*,
                                                     L"\x61\x62\x80\xA0\x63"),
                                 PRUint32((sizeof(L"\x61\x62\x80\xA0\x63") /
                                           sizeof(wchar_t))-1)))

Is the L" failing here because the compiler for some reason has already turned
it into a |const wchar_t[]|?

Can you add the url to the online info on TEXT, or e-mail it to me?

Comment 25

[Roy Yokoyama]

>Is the L" failing here because the compiler for some reason has already >turned
it into a |const wchar_t[]|
Possibly.  Can we change L" to TEXT(" for Win32 only to see if that helps?

Meanwhile here is the online help:
http://msdn.microsoft.com/library/default.asp?url=/library/en-us/intl/unicode_13lb.asp

Comment 26

[jag (Peter Annema)]

Can you try this then:

Index: mozilla/string/public/nsLiteralString.h
===================================================================
RCS file: /cvsroot/mozilla/string/public/nsLiteralString.h,v
retrieving revision 1.13
diff -u -r1.13 nsLiteralString.h
--- mozilla/string/public/nsLiteralString.h     26 Nov 2001 05:59:35 -0000      1.13
+++ mozilla/string/public/nsLiteralString.h     19 Dec 2001 04:04:46 -0000
@@ -64,7 +64,11 @@
 #endif
 
 #ifdef HAVE_CPP_2BYTE_WCHAR_T
+#if defined(XP_WIN) && defined(UNICODE)
+  #define NS_LL(s)                                TEXT(s)
+#else
   #define NS_LL(s)                                L##s
+#endif
   #define NS_MULTILINE_LITERAL_STRING(s)         
nsDependentString(NS_REINTERPRET_CAST(const nsAString::char_type*, s),
PRUint32((sizeof(s)/sizeof(wchar_t))-1))
   #define NS_NAMED_MULTILINE_LITERAL_STRING(n,s)  nsDependentString
n(NS_REINTERPRET_CAST(const nsAString::char_type*, s),
PRUint32((sizeof(s)/sizeof(wchar_t))-1))
 #else


See if this fixes your build.

Comment 27

[Roy Yokoyama]

Your suggestion may work once we make Moz an Unicode app (ie. compile
with -DUNICODE).  However, we are not doing that just yet. 

So your |#ifdef| patch doesn't make any differences; thus we still have a
compiler error.

Make use of |NS_ConvertASCIItoUCS2("\x61\x62\x80\xA0\x63"));|
get around of the compiler error.  I suggest we use
NS_ConvertASCIItoUCS2() for the time being.

Comment 28

[jag (Peter Annema)]

Ah, I thought you were already compiling with -DUNICODE. So yeah, use
NS_ConvertASCIItoUCS2 for now in this one specific place.

Comment 29

[Roy Yokoyama]

Attachment: Fixing a build bustage on non-ASCII OS

here the patch to fix the compiler error on non-English os.
jag: please check this in. thanks

Comment 30

[jag (Peter Annema)]

Comment on attachment 62393 [details] [diff] [review]
Fixing a build bustage on non-ASCII OS

sr=jag

Comment 31

[Roy Yokoyama]

Comment on attachment 62393 [details] [diff] [review]
Fixing a build bustage on non-ASCII OS

I am not sure if this is a valid review (since I put the patch together); but
here is /r=yokoyama

Comment 32

[Katsuhiko Momoi]

*** Bug 115786 has been marked as a duplicate of this bug. ***

Comment 33

[Roy Yokoyama]

Patch "Fixing a build bustage on non-ASCII OS" checked in.

Comment 34

[Katsuhiko Momoi]

roy, if thia has been checked in, do you want to mark it fixed?

Comment 35

[Roy Yokoyama]

momoi-san: I'll leave it to jag until he get rid of all *WithConversion.
This bug has never been closed (by jag) so I am assuming he has 
something else to do. 

Comment 36

[jag (Peter Annema)]

->0.9.9

Comment 37

[jag (Peter Annema)]

-> future