53057 - [API] turn off implicit |CharT*| conversion operators for those string classes that have them

Status: RESOLVED FIXED

(Core :: XPCOM, defect, P3)

Description

[Scott Collins]

I added |.get()| to replace this with an explicit form; went through the tree and 
fixed places where people rely on the implicit form; now just need to remove the 
actual implicit operators.

Why?  Implicit conversion repeatedly leads us down the garden patch to calling 
wrong functions and doing more work than the caller expected often ending in 
multiple copies and conversions of the string.

Comment 1

[Scott Collins]

Attachment: this patch deprecates the implicit character conversion operators, and fixes up the few stragglers that have been checked in since my tree sweep

Comment 2

[Scott Collins]

the patch is a little stale; I'll have to go through and re-find the stragglers,
but this is important.

Comment 3

[Peter "jag" Annema]

Cool. Perhaps I should do this so dbaron can r= and you can a=?

Comment 4

[Scott Collins]

That would be great!

Comment 5

[Peter "jag" Annema]

Attachment: [patch] updated and more fixed. More to come.

Comment 6

[Peter "jag" Annema]

So far only the issues which turned up in my compile (which doesn't include any
tests nor most of the extensions currently). I'll try and do the rest tomorrow.

Assigning to me so I can find it in my buglist :-)

Comment 7

[Peter "jag" Annema]

Attachment: [patch] hopefully everything else

Comment 8

[Peter "jag" Annema]

xpccomponents.cpp and xpcexception.cpp have both been fixed in another bug.

Comment 9

[Peter "jag" Annema]

dbaron, would you mind reviewing this sometime soon? More get-less literal
strings keep sneaking in :-)

Comment 10

[Peter "jag" Annema]

Attachment: [patch] remove operator const CharT*() const from nsLiteralString

Comment 11

[Peter "jag" Annema]

basic_nsLiteralString even.

With the other patches applied I could safely comment that operator out.

Comment 12

[Peter "jag" Annema]

Attachment: [patch] new: nsProfile.cpp, nsWindowWatcher.cpp

Comment 13

[timeless]

r=timeless for 24530,24907

Comment 14

[Scott Collins]

more and more, we see how important this is.

Comment 15

[Peter "jag" Annema]

Attachment: [patch] Remove operator CharT* from all but nsCString, "hide" for now where needed.

Comment 16

[Peter "jag" Annema]

Attachment: [patch] incorporate scc's changes, update, etc.

Comment 17

[Scott Collins]

sr=scc

Comment 18

[Alec Flett]

r=alecf on that last patch
I would like to voice my objection to the global Compare() and ToNewUnicode()
functions, but that can be tackled another time

Comment 19

[Peter "jag" Annema]

Attachment: [patch] remove |operator char*()| from nsCString and fix clients

Comment 20

[Peter "jag" Annema]

So this compiles on linux, windows and mac.

Anyone care to review?

scc, sr=?

I'm hoping to get this one in for 0.8.1 as targetted.

I _doubt_ I'll get the (large!) |nsCString::operator const char*() const|
removal patch in though (in part because I still need to create it).

Comment 21

[Mike Shaver (:shaver emeritus)]

The patch looks like safe, mechanical progress.  I"m glad _I_ didn't have to
type all those filthy NS_CONST_CASTs, lemme tell ya. =)

sr=shaver

Comment 22

[Dan Mosedale (:dmosedale, :dmose)]

At some point, it would be nice if nsLiteralCString and friends could be used in
some of the instances where the constness is just being cast away in order to
insert the const char * into an nsCString.  But this is already an excellent
step forward.

r=[e-mail]

Comment 23

[Dan Mosedale (:dmosedale, :dmose)]

Never mind my cleanup comment; that was me misinterpreting the change to
nsAddrBookUrl::CrackPrintUrl().  r=dmose.

Comment 24

[Peter "jag" Annema]

Moving to 0.9

Comment 25

[Peter "jag" Annema]

Stuff checked in... Next up: operator const char*() const.

Comment 26

[Peter "jag" Annema]

Bah

Comment 27

[Peter "jag" Annema]

Next one then

Comment 28

[Peter "jag" Annema]

Mass move to [e-mail]

Comment 29

[Peter Trudelle]

->0.9.3, not a limbo candidate though

Comment 30

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: jag's patch for |nsCString::operator const char*| and |nsSubsume[C]Str| removal

Comment 31

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: dbaron's preliminary review of jag's changes

Comment 32

[jag (Peter Annema)]

> nsJSEnvironment.cpp:
> 
>   Please use NS_REINTERPRET_CAST.  Or maybe we need inline functions
>   that are PRtoJSUnichar and JStoPRUnichar (or better names) and do
>   the reinterpret cast within and are externally typesafe?  That would
>   probably be a good thing if you can find a good place to put them...

No good place found yet. nsJSUtils.h, nsWWJSUtils.h and some new file for
xpinstall/*? Or perhaps a new file in string/public?


> nsHTMLEditRules.cpp, nsTextEditRules.cpp:
> 
>   ***I need to go over these changes again.***


> inBitmapURI.cpp :
> 
>   Are ContractIDs really preferred to CIDs in C++ code?

Yes, as far as I know they are.

>   inBitmapURI::FormatSpec could really be written better, but you don't
>   have to change it if you don't want...

Done :-)


> COtherElements.h :
> 
>   No need to split to 2 lines.

-  const char* tag=nsHTMLTags::GetStringValue(aTag->mTag);
+  const nsCString& tagStr=nsHTMLTags::GetStringValue(aTag->mTag);
+  const char* tag=tagStr.get();

I guess you're right, but I figured this would make it clearer that you'll need
the nsCString to stay around for the duration of time you need |tag|.


> TestCSSPropertyLookup.cpp :

>   You should be able to use nsDependentString for the first.

I'd have to convert to UCS2 for that, and there's no support for
nsDependentCString (yet).


> mailnews/import/eudora/src/nsEudoraAddress.cpp
> 
>   Are you in a position to remove the pCStr variable?

Yep, done.


> mailnews/mime/src/mimetpla.cpp
> 
>   ***I need to go over these changes again.***
> 
>   No need to create an nsDependentString around an nsXPIDLString
>   ("nsDependentString(citeTagsResultUnichar)").
> 
>   Use |Adopt| rather than |*getter_Copies() =|.

This code was from before the "recent" changes to nsXPIDLString. Changes made
now though :-)

> modules/libpr0n/decoders/icon/nsIconURI.cpp
> 
>   Could you leave the CID?  They're not deprecated, are they?

No, but I'm pretty sure Contract IDs are prefered over Class IDs in most cases.
I can't remember who's stated that though, so let me find out.


> netwerk/mime/src/nsMIMEInfoImpl.cpp
> 
>   ***I need to go over these changes again.***


> nsString2.{h,cpp}
> 
>   ***I need to go over these changes again.***


> xpcom/components/nsComponentManager.cpp
> 
>   Why don't you fix the longstanding bug here and make it
>   entry->location.Equals(registryName) ?

Done.

> xpfe/bootstrap/nsAppRunner.cpp
> 
>   I assume these changes were all for a different bug.

Oops, yes.

Comment 33

[saari (gone)]

0.9.5

Comment 34

[Peter Trudelle]

Mass-moving lower-priority 0.9.5 bugs off to 0.9.6 to make way for remaining
0.9.4/eMojo bugs, and MachV planning, performance and feature work. If you
disagree with any of these targets, please let me know.

Comment 35

[jag (Peter Annema)]

-> 1.0

Comment 36

[jag (Peter Annema)]

Almost done with this one. -> 0.9.7

Comment 37

[jag (Peter Annema)]

Checked in the removal of |operator const char*() const| from nsCString.

Do we want to do nsXPIDLString and nsXPIDLCString?

Comment 38

[Roland Mainz]

It looks that the latest checkin shot XLib toolkit dead:
-- snip --
nsDragService.cpp
Building deps for
../../../../../../../../home/mozilla/src/2001-11-14-08-trunk/mozilla/widget/src/xlib/nsDragService.cpp
/opt/SUNWspro/bin/CC -xarch=v9 -o nsDragService.o -c -DOSTYPE=\"SunOS5\"
-DOSARCH=\"SunOS\" -DOJI -D_IMPL_NS_WIDGET
-I../../../../../../../../home/mozilla/src/2001-11-14-08-trunk/mozilla/widget/src/xlib/../xpwidgets
-I../../../../../../../../home/mozilla/src/2001-11-14-08-trunk/mozilla/widget/src/xlib 
-I../../../dist/include/xpcom -I../../../dist/include/string
-I../../../dist/include/appshell -I../../../dist/include/gfx
-I../../../dist/include/layout -I../../../dist/include/content
-I../../../dist/include/dom -I../../../dist/include/js
-I../../../dist/include/xlibrgb -I../../../dist/include/timer
-I../../../dist/include/uconv -I../../../dist/include/pref
-I../../../dist/include/webshell -I../../../dist/include/htmlparser
-I../../../dist/include/view -I../../../dist/include/necko
-I../../../dist/include/gfx2 -I../../../dist/include/widget
-I../../../dist/include
-I/shared/bigtmp2/mozilla/2001-11-14-08-trunk/objdir_ws6_xlib_sparcv9/dist/include/nspr     
-I/usr/openwin/include   -KPIC  -I/usr/openwin/include -mt -O  -DDEBUG
-DDEBUG_mozilla -DTRACING -g -I/usr/openwin/include -I/usr/openwin/include
-DMOZILLA_CLIENT -DBROKEN_QSORT=1 -DNSCAP_DISABLE_DEBUG_PTR_TYPES=1
-DD_INO=d_ino -DSTDC_HEADERS=1 -DHAVE_ST_BLKSIZE=1 -DHAVE_INT16_T=1
-DHAVE_INT32_T=1 -DHAVE_INT64_T=1 -DHAVE_UINT=1 -DHAVE_UINT_T=1
-DHAVE_UINT16_T=1 -DHAVE_64BIT_OS=1 -DHAVE_DIRENT_H=1 -DHAVE_SYS_BYTEORDER_H=1
-DHAVE_MEMORY_H=1 -DHAVE_UNISTD_H=1 -DHAVE_NL_TYPES_H=1 -DHAVE_SYS_STATVFS_H=1
-DHAVE_SYS_STATFS_H=1 -DHAVE_SYS_VFS_H=1 -DHAVE_SYS_MOUNT_H=1 -DHAVE_LIBM=1
-DHAVE_LIBDL=1 -DHAVE_LIBSOCKET=1 -D_REENTRANT=1 -DHAVE_RANDOM=1
-DHAVE_STRERROR=1 -DHAVE_LCHOWN=1 -DHAVE_FCHMOD=1 -DHAVE_SNPRINTF=1
-DHAVE_LOCALTIME_R=1 -DHAVE_STATVFS=1 -DHAVE_MEMMOVE=1 -DHAVE_RINT=1
-DHAVE_NL_LANGINFO=1 -DHAVE_STRTOK_R=1 -DHAVE_IOS_BINARY=1 -DHAVE_CPP_EXPLICIT=1
-DHAVE_CPP_SPECIALIZATION=1 -DHAVE_CPP_MODERN_SPECIALIZE_TEMPLATE_SYNTAX=1
-DHAVE_CPP_PARTIAL_SPECIALIZATION=1 -DHAVE_CPP_ACCESS_CHANGING_USING=1
-DHAVE_CPP_AMBIGUITY_RESOLVING_USING=1 -DHAVE_CPP_NAMESPACE_STD=1
-DHAVE_CPP_UNAMBIGUOUS_STD_NOTEQUAL=1 -DHAVE_CPP_NEW_CASTS=1
-DHAVE_CPP_DYNAMIC_CAST_TO_VOID_PTR=1 -DNEED_CPP_UNUSED_IMPLEMENTATIONS=1
-DHAVE_I18N_LC_MESSAGES=1 -DMOZ_DEFAULT_TOOLKIT=\"xlib\" -DMOZ_WIDGET_XLIB=1
-DMOZ_ENABLE_XREMOTE=1 -DMOZ_X11=1 -DIBMBIDI=1 -DSUNCTL=1 -DACCESSIBILITY=1
-DMOZ_MATHML=1 -DMOZ_SVG=1 -DMOZ_LOGGING=1 -DDETECT_WEBSHELL_LEAKS=1
-DMOZ_USER_DIR=\".mozilla\" -DMOZ_XUL=1 -DINCLUDE_XUL=1 -DNS_MT_SUPPORTED=1
-DUSE_IMG2=1 -DMOZ_DLL_SUFFIX=\".so\" -DXP_UNIX=1 -DUNIX_ASYNC_DNS=1
-DJS_THREADSAFE=1 -DNS_PRINT_PREVIEW=1 -DMOZ_REFLOW_PERF=1
-DMOZ_REFLOW_PERF_DSP=1 
../../../../../../../../home/mozilla/src/2001-11-14-08-trunk/mozilla/widget/src/xlib/nsDragService.cpp
"../../../../../../../../home/mozilla/src/2001-11-14-08-trunk/mozilla/widget/src/xlib/nsDragService.cpp",
line 212: Error: Cannot cast from nsCAutoString to const char*.
"../../../../../../../../home/mozilla/src/2001-11-14-08-trunk/mozilla/widget/src/xlib/nsDragService.cpp",
line 213: Error: Cannot cast from nsCAutoString to const char*.
2 Error(s) detected.
-- snip --

I'll file a fix for that ...

Comment 39

[Roland Mainz]

Attachment: Fix for the Xlib toolkit bustage (2001-11-14-08-trunk)

Comment 40

[Roland Mainz]

Can someone r= the patch and check it in, please ?

Comment 41

[jag (Peter Annema)]

Comment on attachment 57924 [details] [diff] [review]
Fix for the Xlib toolkit bustage (2001-11-14-08-trunk)

r=jag

Comment 42

[jag (Peter Annema)]

Need to discuss what to do about nsXPIDLC?String with scc and dbaron. Moving to
0.9.since this will most likely not get fixed in 0.9.7

Comment 43

[jag (Peter Annema)]

-> future

Comment 44

[jag (Peter Annema)]

filter keyword: nothingnessless

Comment 45

[jag (Peter Annema)]

operator const char_type* has been removed for everything but nsXPIDLC?String. Going to mark this bug fixed. I'll see about filing a new bug to deal with that case.