17641 - Enhance nsString

Status: RESOLVED FIXED

(Core :: XPCOM, defect, P3)

Description

[Ben Bucksch (:BenB)]

I see (1.) (single) char constructors and (2.) making operators more symetric as
useful.

Example for 2.: |"Hello" + myString| should work as well as |myString +
"Hello"|.

Comment 1

[Ben Bucksch (:BenB)]

Attachment: Fix for wrong nsString(PRUnichar) usage

Comment 2

[Ben Bucksch (:BenB)]

If I'm not completely wrong, this was a try to convert an int to a string using
the PRUnichar constructor. Fixed.

This fix is the result from an (expected) side effect of the new char
constructor: most wrong int-constructor usages, like trying to set an initial
capacity, get amibitious now and produce errors.

Comment 3

[Ben Bucksch (:BenB)]

Attachment: LOL, I think, I made the same error. Using radix explicitly now.

Comment 4

[Ben Bucksch (:BenB)]

Attachment: Explicit conversion to [PRUni]char

Comment 5

[Ben Bucksch (:BenB)]

Attachment: Try 1 - nsString2.h

Comment 6

[Ben Bucksch (:BenB)]

Attachment: Try 1 - nsString2.cpp

Comment 7

[Ben Bucksch (:BenB)]

Attachment: Try 1 - nsString.h

Comment 8

[Ben Bucksch (:BenB)]

Attachment: Try 1 - nsString.cpp

Comment 9

[Ben Bucksch (:BenB)]

This is the first attempt to the proposal. It implements several friend
operators (namely ==, !=, <, >, <=, >=, +(the latter only for nsString, I forgot
nsCString)) and [PRUni]char constructors.

The patches before fix problems I had during the clobber build, but everything
should be OK now. I'll add friend operator+ for nsCString and do one more
clubber to test this and the following change, but I don't expect problems.

One error I came across I didn't understand. C++'s implicit conversions give me
headache.

nsNntpService.cpp: In method `nsresult
nsNntpService::DetermineHostForPosting(class
 nsCString &, const char *)':
nsNntpService.cpp:434: ambiguous overload for `nsCString & != nsCAutoString &'
nsNntpService.cpp:434: candidates are: operator !=(const char *, const char *)
<builtin>
nsNntpService.cpp:434:  operator !=(char *, char *) <builtin>
nsNntpService.cpp:434:  operator !=(const char *, const char *) <bui
ltin>
nsNntpService.cpp:434:  operator !=(const char *, const char *) <bui
ltin>
nsNntpService.cpp:434:  operator !=(char *, char *) <builtin>
nsNntpService.cpp:434:  operator !=(char *, char *) <builtin>
../../../dist/include/nsString.h:628:  nsCString::operator !=(const nsStr &)
const
../../../dist/include/nsString.h:629:  nsCString::operator !=(const char *)
const
../../../dist/include/nsString2.h:688:  operator !=(const char *, const nsString
&)
../../../dist/include/nsString.h:631:  operator !=(const char *, const nsCString
&)
nsNntpService.cpp: In method `nsresult
nsNntpService::ConvertNewsgroupsString(const
 char *, char **)':
nsNntpService.cpp:578: ambiguous overload for `nsCAutoString & != nsCAutoString
&'
nsNntpService.cpp:578: candidates are: operator !=(const char *, const char *)
<bui
ltin>
nsNntpService.cpp:578:   operator !=(char *, char *) <builtin>
nsNntpService.cpp:578:   operator !=(const char *, const char *) <builtin>
nsNntpService.cpp:578:   operator !=(const char *, const char *) <builtin>
nsNntpService.cpp:578:   operator !=(char *, char *) <builtin>
nsNntpService.cpp:578:   operator !=(char *, char *) <builtin>
../../../dist/include/nsString.h:628:    nsCString::operator !=(const nsStr &)
const
../../../dist/include/nsString.h:629:    nsCString::operator !=(const char *)
const
../../../dist/include/nsString2.h:688:    operator !=(const char *, const
nsString &)
../../../dist/include/nsString.h:631:   operator !=(const char *, const
nsCString &)
make[4]: *** [nsNntpService.o] Error 1
[Some whitespace deleted]

I fixed this by adding
PRBool nsCString::operator!=(const nsCString& S) const {return
PRBool(Compare(S)!=0);}  //DELETEME: Hack!
but I would feel much better, if I knew, why the occurs. If anyone can give me
hints (e.g. why the char* operators are triggered at all, since there're no
conversion functions to char*), I would be thankful. At the moment, I'm reading
the spec, but I don't know, if I will find the solution.

Comment 10

[Ben Bucksch (:BenB)]

Attachment: Try 2 - nsString.tar.gz

Comment 11

[Ben Bucksch (:BenB)]

Deleting valeski - he should have seen the patch now.
valeski: if you're still interested, readd yourself.

Comment 12

[Ben Bucksch (:BenB)]

I added the remaining |operator+|s, changed the operators to non-friends adn did
one more clobber test - no problems.

But the (theorethical) problem with ambitious operators still exists :-(.

Comment 13

[Ben Bucksch (:BenB)]

Giving away

Comment 14

[Brendan Eich [:brendan]]

Rickg owns nsString, so I'm passing the buck.

/be

Comment 15

[rickg]

I don't get Brendans remark, but I'll hang onto this patch until the next
revision of nsString. After beta1, we're planning to add cow semantics.

Comment 16

[Brendan Eich [:brendan]]

If you View Bug Activity, you'll see that BenB ([e-mail]) assigned
this bug to me, but I don't own nsString, so I passed the bug, er, buck.  It
felt like I was passing the buck, anyway -- and bugzilla requires me to enter a
comment when reassigning.  Sorry for the confusion.

/be

Comment 17

[Blake Ross]

reopening and marking Future...

Comment 18

[Ben Bucksch (:BenB)]

->scc

scc, if this is already fixed with the new string API, mark it fixed.

Comment 19

[Scott Collins]

The areas effected by these patches are substantially changed in the last year.  
We no longer allow implicit converting constructors (so those changes are now 
covered).  |Append| of an integer has been replaced with |AppendInt| so that's 
covered.  |nsSubsumeString| is deprecated (e.g., in favor of
|nsPrommiseConcatenation|).  And the comparisons for readables have been very 
carefully crafted compile everywhere and give us what we need.  So, after 
analysis of the patches supplied in this bug, I think we've got it covered.

Marking FIXED as per Ben's request.

Comment 20

[Pulsebot]

Pushed by ffxbld-merge:
https://hg.mozilla.org/releases/mozilla-beta/rev/ecc83bfdbd11
[fenix] For https://github.com/mozilla-mobile/fenix/issues/17641 Fix Intermittent test addPredefinedSearchEngineTest