113733 - nsXPIDLCString doesn't have an assignment operator

Status: RESOLVED FIXED

(Core :: XPCOM, defect)

Description

[Bradley Baetz (:bbaetz)]

See netwerk/protocol/http/src/nsHttpChannel.cpp:149, which warns:

warning: using synthesized `nsXPIDLCString &nsXPIDLCString::operator= (const
nsXPIDLCString &)' for copy assignment where cfront would use `nsXPIDLCString
&nsXPIDLCString::operator= (const nsACString &)'

This warning is correct; XPIDLCString's do not have an assignment operator, and
that line will tirgger incorrect behaviour (if not now, then definately once
sharing happens)

nsSharableCString does have an operator=(nsACString&), but stroustrup 12.2.3
says: ".. assignment operators are not inherited". This makes sense, for the
same reason that constructors aren't inherited.

The other string classes probably need checking, too

Comment 1

[jag (Peter Annema)]

Attachment: Let it have operator=

Comment 2

[jag (Peter Annema)]

dbaron: assuming that a generated operator= will do a bitwise copy, that would
rather mess with the refcounting, right? Could that be the cause of some of the
problems we've been (and still are?) seeing with that?

Comment 3

[jag (Peter Annema)]

And as bbaetz just pointed out, we should probably copy this in
nsSharableString.

Comment 4

[Alec Flett]

Comment on attachment 67422 [details] [diff] [review]
Let it have operator=

sure. if jag sez so :)
sr=alecf

Comment 5

[Bradley Baetz (:bbaetz)]

Attachment: sharable string too

Comment 6

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

Comment on attachment 69627 [details] [diff] [review]
sharable string too

r=dbaron, although I wonder if we really want the latter two (|const
char_type*| and |char_type|) on the XPIDL strings.

Comment 7

[jag (Peter Annema)]

I agree with dbaron, let's leave out the |const CharT*| and |CharT| operator=.

Comment 8

[Scott Collins]

giving up ancient string bugs to the new string owner.  jag, you'll want to sort
through these and see which ones still apply and go with or against the
direction in which you intend strings evolve

Comment 9

[[e-mail]]

Attachment: update to trunk

It looks like bug 122892 is still waiting on this fix, so as far as I can tell
it'd still be nice to get this in (please correct me if I'm wrong).

I've updated the patch to trunk, but in addition I had to remove the |const
promise_type&| copy constructor since |promise_type| no longer exists.

Comment 10

[[e-mail]]

Comment on attachment 127222 [details] [diff] [review]
update to trunk

dbaron, mind taking a look? (I'd like to hold off on requesting sr=jag until
the patch has r+).

thanks!

Comment 11

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

Comment on attachment 127222 [details] [diff] [review]
update to trunk

Some of the comments seem a bit confused -- stating the reason in two different
ways.  Also, why do we need two versions -- isn't the |operator=(const
abstract_string_type&)| sufficient?

Comment 12

[[e-mail]]

I'm not sure - XPIDLString/SharableString have had an |operator=(const
abstract_string_type&)| since this bug was filed, but this warning still exists.
I'm not sure why the compiler would synthesize |operator=(const
nsXPIDLCString&)| when it has the former available.

Any idea what's going on here?

Comment 13

[Bradley Baetz (:bbaetz)]

dwitte: operator= isn't inherited. See my original comment #0.

Comment 14

[[e-mail]]

Oh, wait - no, I can see why we'd need |operator=(const self_type&)|. I presume
that declaring it private and leaving it unimplemented would break; since the
compiler wouldn't know to fall back on the provided |operator=(const
abstract_string_type&)|. So, isn't providing the self_type operator on
XPIDLString/SharableString the right thing to do here?

(just saw your comment bbaetz... I understand now ;)

Comment 15

[Bradley Baetz (:bbaetz)]

dbaron: The issue is that the compiler synthesises a |self_type&
operator=(self_type&)|. Its then the best match to use.

I can't find my copy of the c++ std, but in the draft at
http://www.csci.csusb.edu/dick/c++std/cd2/over.html, see the example at 13.5.3/2

Comment 16

[[e-mail]]

Attachment: fix comments

Comment 17

[[e-mail]]

Comment on attachment 127348 [details] [diff] [review]
fix comments

same thing as before, but with consistent comments.

Comment 18

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

Comment on attachment 127348 [details] [diff] [review]
fix comments

r=dbaron if you add a comment before all the |const self_type&| versions that
says "// prevent the compiler from synthesizing an operator=".

(I was thinking that the compiler would only synthesize if none were provided
-- like constructors -- but it's clear (from C++ 12.8 clauses 9 and 10) that
I'm wrong.)

Comment 19

[[e-mail]]

Attachment: fix comments v2

Comment 20

[[e-mail]]

Comment on attachment 127390 [details] [diff] [review]
fix comments v2

carrying over r=dbaron.

Comment 21

[jag (Peter Annema)]

Comment on attachment 127390 [details] [diff] [review]
fix comments v2

Though you're technically preventing the compiler from synthesizing the copy
operator= (though I think we usually only use the word "prevent" when it's in a
private section with no implementation), what you're really doing here is
something like "// providing copy operator= since the synthesized one doesn't
do the right thing", though really that should just be implied and no comment
should be needed. See e.g. nsAString.h's operator= comment.

Comment 22

[[e-mail]]

Checked in, with comments stolen from nsAString.h.