Closed
Bug 100476
Opened 23 years ago
Closed 23 years ago
X.ToNewXXX() -> ToNewXXX(X) where XXX in (Unicode, CString, UTF8String)
Categories
(Core :: XPCOM, defect)
Core
XPCOM
Tracking
()
RESOLVED
FIXED
People
(Reporter: jag+mozilla, Assigned: jag+mozilla)
Details
Attachments
(2 files)
569.06 KB,
patch
|
Details | Diff | Splinter Review | |
570.65 KB,
patch
|
dbaron
:
review+
|
Details | Diff | Splinter Review |
Assignee | ||
Comment 1•23 years ago
|
||
Here are comments on the part up to but not including nsAbAutoCompleteSession.cpp: (As a side note, I really don't like the |ToNewUnicode| from char* or nsACString& since it isn't clear whether it's converting from ASCII or UTF8.) There are places where you're using NS_ConvertUCS2toUTF8 to pass to printf (to fix debugging code that leaked). Maybe you should write and use NS_LossyConvertUCS2toASCII (which would be a very simple nsCAutoString wrapper)? Index: embedding/browser/photon/src/WebBrowserContainer.cpp + moz->rightClickUrl = strdup( ToNewCString(rightClickUrl) ); redundant and leaky (not that it wasn't before, but...) Index: extensions/xmlextras/base/src/nsXMLHttpRequest.cpp Remove these comments: // XXX There is no ASCII->Unicode decoder? // XXX How to do this without double allocation/copy? Index: gfx/src/photon/nsFontMetricsPh.cpp - PRUnichar* str = ((nsString*) he->key)->ToNewUnicode(); + PRUnichar* str = ToNewUnicode(*((nsString*) he->key)); No need to over-parenthesize, but NS_STATIC_CAST would be good too (and avoid the parens issue). Index: gfx/src/photon/nsRenderingContextPh.cpp - char* tail=(char*)strTail.ToNewUnicode(); + char* tail=(char*)ToNewUnicode(strTail); Um? What's going on here? Make that an NS_REINTERPRET_CAST at the very least, with big XXX comments. Index: js/src/xpconnect/src/xpcconvert.cpp Could you just use the NS_ConvertASCIItoUCS2 inline rather than making a new variable?
> Index: gfx/src/photon/nsFontMetricsPh.cpp
> - PRUnichar* str = ((nsString*) he->key)->ToNewUnicode();
> + PRUnichar* str = ToNewUnicode(*((nsString*) he->key));
> No need to over-parenthesize, but NS_STATIC_CAST would be good too (and
> avoid the parens issue).
(But for that it would need to be NS_STATIC_CAST(const nsString*, he->key) .)
Assignee | ||
Comment 4•23 years ago
|
||
I'll write the lossy converter. About ToNewUnicode from a char* / nsACString being unclear what encoding it's converting from, yeah, maybe we should add a second parameter to indicate which encoding we're converting from? For now, it's assumed to be ASCII, and nothing has changed there. From your comment it sounds like this is an issue we could address seperately. I've adddressed your other comments and fixed the code. I'll explicitely mention this here: /* XXXjag this code looks very bogus to me, but I've no idea what they're * trying to accomplish here. At my best guess they're trying to do this: * char* text = ToNewCString(nsDependentCString(aString, aLength) + * NS_LITERAL_CSTRING("M.")); * text[aLength + 1] = '\0'; // overwrites the '.' above */ nsCString strTail("M"); char* tail=(char*)ToNewUnicode(strTail); PRUint32 tailLength=strlen(tail); char* text = (char*) nsMemory::Alloc(aLength + tailLength + 2); PRUint32 i; for(i=0;i<aLength;i++) text[i]=aString[i]; for(i=0;i<tailLength;i++) text[aLength+i] = tail[i]; text[aLength+tailLength]='\0'; text[aLength+tailLength+1]='\0'; Now, I know there is some leaky string code in embedding/browser/XXX/src where XXX isn't one of the mainstream platforms. Unless the fix was obvious (bar the few I mixed like the one you pointed out above), I decided not to waste too much time trying to "fix" it, since it would take me a fair while to find out if my suspiscion is correct, but even a longer time to get the code to compile, whereas I know my simple string substitution to still compile.
I'd rather see the ToNewUnicode(const char*) and ToNewUnicode(nsACString&) be split into UTF8ToNewUnicode and ASCIIToNewUnicode, or something like that...
You had to use the following pattern repeatedly: ToNewUnicode(NS_ConvertUTF8toUCS2(name)) which suggests to me we need a |UTF8ToNewUnicode| (not for this checkin, though, but soon). I do expect NS_LossyConvertUCS2toASCII for this checkin though. It's trivial, since it's just an AssignWithConversion, and you shouldn't be printing UTF8 to the console. File a bug against nsAbUtils.cpp for having an if-else that do the same thing. Dare I guess |copyElements| is always true? I expect the nsMsgSendLater.cpp change won't compile. Please let me know why it does. nsEudoraAddress.cpp: You added a shadowing variable uniStr without removing the old (unneeded) one. mimemoz2.cpp |resultString| should be |const char*|. You need a second |else /* !stringBundle */| to assign tempString = nsCRT::strdup(resultString). nsIconChannel.cpp: - *((char **)getter_Copies(filePath)) = formattedFileExt.ToNewCString(); // yuck...shove it into our xpidl string... + filePath.Adopt(ToNewCString(formattedFileExt)); // yuck...shove it into our xpidl string... You can remove the comment now. :-) nsAccount.cpp: + char* lvalue = ToNewCString(v); + + PL_strcpy(IspLocation[LocCount], lvalue); + Can just use v.get() and remove the |delete[]| which is an FMM anyway. nsNSSCertificate.cpp: + //nsXPIDLCString tmpstrl tmpstr.Adopt(CERT_Hexify(&mCert->serialNumber, 1)); should be ';' instead of 'l'. ==== I skipped the changes in nsString2.cpp -- need to come back to these. ==== (And really, can we get rid of NS_ConvertUTF8toUCS2(char)?) photon/nsMenu.cpp: + char *st r =ToNewCString(mLabel); really? nsPersistentProperties.cpp: - cout << "will add " << aKey.ToNewCString() << "=" << aNewValue.ToNewCString() << endl; + cout << "will add " << ToNewCString(aKey) << "=" << ToNewCString(aNewValue) << endl; These should use NS_LossyConvertUCS2toASCII (with the XXX comment removed). nsAppRunner.cpp: These changes are in the wrong tree! nsGlobalHistory.cpp: - value = valueStr.ToNewUnicode(); - len = nsCRT::strlen((PRUnichar*)value); + value = ToNewUnicode(valueStr); + len = valueStr.Length() * sizeof(PRUnichar); Skip the |* sizeof(PRUnichar)|. I've done up to but not including nsUrlBarHistory.cpp, and skipped nsString2.cpp, as noted above.
The foo.Adopt(ToNewXXX(Bar)); pattern is bad because it defeats sharing. Does nsXPIDL[C]String::Assign work? Can we make it work? nsInstall.cpp: - JSString* propValJSStr = JS_NewUCStringCopyZ(cx, (jschar*) valCStr); + JSString* propValJSStr = JS_NewUCStringCopyZ(cx, NS_STATIC_CAST(const jschar*, pVal.get())); some platforms will require an NS_REINTERPRET_CAST Also, use a .get() on keyCStr in the JS_SetProperty call. Now I just have to look at nsString2.cpp.
nsString2.cpp: the iterators (ns{Reading,Writing}Iterator) ought to have a fragment_type so that you don't have to use nsReadableFragment<char> explicitly. + nsReadableFragment<char> frag(start.fragment()); Maybe const nsReadableFragment<char>& frag = start.fragment(); would be better? How about an NS_ASSERTION(count == mLength, "calculator calculated incorrect length"); at the end of nsConvertUTF8toUCS2::Init? I think we need to look at the error-handling of NS_ConvertUTF8toUCS2 carefully since we can expect it to be called on input from the network (I would think, anyway, although perhaps not). + else + { + ucs4 -= 0x00010000; + *mBuffer++ = 0xD800 | (0x000003FF & (ucs4 >> 10)); + *mBuffer++ = 0xDC00 | (0x000003FF & ucs4); + } Is this UCS2 or UTF-16 or what?
Assignee | ||
Comment 9•23 years ago
|
||
>You had to use the following pattern repeatedly: > ToNewUnicode(NS_ConvertUTF8toUCS2(name)) >which suggests to me we need a |UTF8ToNewUnicode| (not for this checkin, >though, but soon). I do expect NS_LossyConvertUCS2toASCII for this >checkin though. It's trivial, since it's just an >AssignWithConversion, and you shouldn't be printing UTF8 to the console. NS_LossyConvertUCS2toASCII added. >File a bug against nsAbUtils.cpp for having an if-else that do the same >thing. Dare I guess |copyElements| is always true? No, it's not always true. Filed, bug 100743. >I expect the nsMsgSendLater.cpp change won't compile. Please let me >know why it does. Because |field| is a nsMsgCompFields, which has ::SetFrom(const char*, ...) >nsEudoraAddress.cpp: > > You added a shadowing variable uniStr without removing the old > (unneeded) one. Fixed. >mimemoz2.cpp > > |resultString| should be |const char*|. Fixed > You need a second |else /* !stringBundle */| to assign > tempString = nsCRT::strdup(resultString). Fixed, by moving that out of the |if (stringBundle)| and changing it to if (!tempString) tempString = nsCRT::strdup(resultString); >nsIconChannel.cpp: > >- *((char **)getter_Copies(filePath)) = formattedFileExt.ToNewCString(); // >yuck...shove it into our xpidl string... >+ filePath.Adopt(ToNewCString(formattedFileExt)); // yuck...shove it into >our xpidl string... > >You can remove the comment now. :-) Removed, but it's still kinda yucky though ;-) >nsAccount.cpp: > > + char* lvalue = ToNewCString(v); > + > + PL_strcpy(IspLocation[LocCount], lvalue); > + > >Can just use v.get() and remove the |delete[]| which is an FMM anyway. I think so. >nsNSSCertificate.cpp: >+ //nsXPIDLCString tmpstrl tmpstr.Adopt(CERT_Hexify(&mCert->serialNumber, 1)); > >should be ';' instead of 'l'. Fixed >==== I skipped the changes in nsString2.cpp -- need to come back to >these. ==== > >(And really, can we get rid of NS_ConvertUTF8toUCS2(char)?) Maybe. >photon/nsMenu.cpp: > >+ char *st r =ToNewCString(mLabel); > >really? You've uncovered my secret plot to break the photon code. I'll fix it *sigh*. >nsPersistentProperties.cpp: > >- cout << "will add " << aKey.ToNewCString() << "=" << aNewValue.ToNewCString() ><< endl; >+ cout << "will add " << ToNewCString(aKey) << "=" << ToNewCString(aNewValue) ><< endl; > >These should use NS_LossyConvertUCS2toASCII (with the XXX comment >removed). Done. >nsAppRunner.cpp: > > These changes are in the wrong tree! Yes they are. Thanks for catching that, would've been a regression. >nsGlobalHistory.cpp: > >- value = valueStr.ToNewUnicode(); >- len = nsCRT::strlen((PRUnichar*)value); >+ value = ToNewUnicode(valueStr); >+ len = valueStr.Length() * sizeof(PRUnichar); > >Skip the |* sizeof(PRUnichar)|. A little lower, very similar code, also dealing with PRUnichar*, does: 1281 PRUnichar* p; 1282 rv = name->GetValue(&p); 1283 if (NS_FAILED(rv)) return rv; 1284 1285 len = nsCRT::strlen(p) * sizeof(PRUnichar); 1286 value = p; This len is used in nsGlobalHistory::URLEnumerator, which uses it with mork, where they want to know the size in bytes. I'm just fixing a bug while I'm there :-)
Assignee | ||
Comment 10•23 years ago
|
||
Btw, the NS_ConvertXXXtoYYY part of the patch really belongs to a different bug. Wanna move it there (and not delay this patch any further)? >nsString2.cpp: > >the iterators (ns{Reading,Writing}Iterator) ought to have a fragment_type so >that you don't have to use nsReadableFragment<char> explicitly. Next pass? New bug? > + nsReadableFragment<char> frag(start.fragment()); > >Maybe > > const nsReadableFragment<char>& frag = start.fragment(); > >would be better? I could use that. >How about an NS_ASSERTION(count == mLength, "calculator calculated >incorrect length"); at the end of nsConvertUTF8toUCS2::Init? Done. >I think we need to look at the error-handling of NS_ConvertUTF8toUCS2 >carefully since we can expect it to be called on input from the network >(I would think, anyway, although perhaps not). I should do that. >+ else >+ { >+ ucs4 -= 0x00010000; >+ *mBuffer++ = 0xD800 | (0x000003FF & (ucs4 >> 10)); >+ *mBuffer++ = 0xDC00 | (0x000003FF & ucs4); >+ } > >Is this UCS2 or UTF-16 or what? I'll come back to you on this one :-)
Assignee | ||
Comment 11•23 years ago
|
||
Assignee | ||
Comment 12•23 years ago
|
||
Hmm, looks like I didn't create another patch after updating string/obsolete/*. Anyway, that's bug 73292 so I'll put that there.
Status: NEW → ASSIGNED
Comment on attachment 50715 [details] [diff] [review] update (still including the string/obsolete/* changes for a different patch) nsDocShell.cpp: random piece of whitespace introduced nsGfxListControlFrame.cpp: You're using NS_ConvertUCS2toUTF8 for a printf - convert to NS_LossyConvertUCS2toASCII nsAbUtils.cpp: Could you just do the minimal change to this file and put the other changes in the separate bug already filed (I think -- see above). nsString.cpp: You can optimize with a SetCapacity if you want. r=dbaron, except on the changes that are part of bug 73292 which are revised there and which I will review there
Attachment #50715 -
Flags: review+
Assignee | ||
Comment 14•23 years ago
|
||
Checked in.
Status: ASSIGNED → RESOLVED
Closed: 23 years ago
Resolution: --- → FIXED
Updated•3 years ago
|
Component: String → XPCOM
You need to log in
before you can comment on or make changes to this bug.
Description
•