warning C4522: 'Relation': multiple assignment operators specified

RESOLVED FIXED in mozilla27

Status

()

Core
Disability Access APIs
RESOLVED FIXED
6 years ago
4 years ago

People

(Reporter: surkov, Assigned: dholbert)

Tracking

(Blocks: 2 bugs, {access})

unspecified
mozilla27
x86
Windows 7
access
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite -

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [build_warning])

Attachments

(1 attachment, 1 obsolete attachment)

(Reporter)

Description

6 years ago
Bug 641838 introduced compile warning:

Relation.h(151) : warning C4522: 'Relation': multiple assignment operators specified

Updated

6 years ago
Blocks: 187528

Updated

6 years ago
Whiteboard: [build_warning]
This is due to the fact that we have two "operator =" methods declared in Relation.h.  The first one is public (with a definition), and the second one is private (and is undefined).

The private one is unused (it must be, because we don't define it anywhere), so we presumably should just drop that declaration.
Created attachment 820008 [details] [diff] [review]
fix v1
Assignee: nobody → dholbert
Status: NEW → ASSIGNED
Attachment #820008 - Flags: review?(surkov.alexander)
(In reply to Daniel Holbert [:dholbert] from comment #1)
> This is due to the fact that we have two "operator =" methods declared in
> Relation.h.  The first one is public (with a definition), and the second one
> is private (and is undefined).
> 
> The private one is unused (it must be, because we don't define it anywhere),
> so we presumably should just drop that declaration.

The real right fix here would be for me to just get around to move constructorifying all of this.

Are you sure that if you remove the private declaration the compiler won't start autogenerating it, and then maybe using it instead of the manually declared one?
(Reporter)

Updated

4 years ago
Attachment #820008 - Flags: superreview?(neil)
Hold on. Let's step back.

Are you saying there's value to declaring *both* a public *and* a private operator=, with the same function signature?  (And in this case, only providing a definition for one of the two?)

I thought (maybe incorrectly) that classes could only have one such assignment operator - not a "public flavor and a private flavor".  So by my understanding, the private declaration is bogus and adds nothing. (and the compiler warning seems to support my impression that we're being redundant)

But maybe there's some C++ arcana here that I don't know about?
(I'm trying to interpret some meaning from the existing code -- are we perhaps trying to say "other things can reassign me (using the public method), but I can't reassign myself (because I'll get the private method which is left undefined)"?  If so: that seems like an odd restriction to place, and I'm also not sure this actually accomplishes that.)

I'm probably wrong about some part of this, or missing something; please enlighten me. :)  (Or perhaps you misunderstood and it's really as simple as I thought it was?)
(Reporter)

Comment 6

4 years ago
Comment on attachment 820008 [details] [diff] [review]
fix v1

moving request to Trev since he's on the bug (and iirc an author of the code).
Attachment #820008 - Flags: review?(surkov.alexander) → review?(trev.saunders)
Comment on attachment 820008 [details] [diff] [review]
fix v1

Thanks. (I also don't see any reason this needs superreview, though neil's free to chime in if he'd like. IIUC superreview is just for architectural changes & changes to interfaces, mostly.)
Attachment #820008 - Flags: superreview?(neil)
silly midair stole my comment the first time...

(In reply to Daniel Holbert [:dholbert] from comment #4)
> Hold on. Let's step back.
> 
> Are you saying there's value to declaring *both* a public *and* a private
> operator=, with the same function signature?  (And in this case, only
> providing a definition for one of the two?)

not quiet the same signature, the public one takes Relation& the private one const Relation&

However I'm pretty sure the public one taking Relation& is dangerous and should be removed, I'm pretty sure calling it ends the world so it can probably be deleted I hope.  Or actually maybe not because of operator = on nsAutoPtr being weird...

> I thought (maybe incorrectly) that classes could only have one such
> assignment operator - not a "public flavor and a private flavor".  So by my
> understanding, the private declaration is bogus and adds nothing. (and the
> compiler warning seems to support my impression that we're being redundant)

what this code was trying to do was force that copying happens through RelationCopyHelper so that these objects code uniquely own something and be returned while owning something.

They should have had a private operator = (const Relation&) and a public operator = (RelationCopyHelper&)

if you remove the public operator = taking Relation& does it still compile and not warn?

> But maybe there's some C++ arcana here that I don't know about?

I don't think so just silly code (though I hadn't noticed that till now)
Ohhh, sneaky, I missed the "const" difference. This makes slightly more sense now. :)

(In reply to Trevor Saunders (:tbsaunde) from comment #8)
> if you remove the public operator = taking Relation& does it still compile
> and not warn?

I'll give that a try tomorrow -- I've rebooted out of Windows now (and Windows is where I happened to run into this problem).
Flags: needinfo?(dholbert)
(In reply to Trevor Saunders from comment #3)
> Are you sure that if you remove the private declaration the compiler won't
> start autogenerating it, and then maybe using it instead of the manually
> declared one?
MSVC only seems to get this right in the case of a non-const assignment operator, which fortunately is exactly the case here. (It gets it wrong in the case of a move assignment operator or a constructor, although gcc gets it right in both cases.)

(In reply to Daniel Holbert from comment #4)
> Are you saying there's value to declaring *both* a public *and* a private
> operator=, with the same function signature?  (And in this case, only
> providing a definition for one of the two?)
No, the private one takes a const parameter, so the signature is different. But the MSVC compiler issues the warning because the difference is so subtle that you missed it!
(Yeah, my comment is late, I was too busy trying to get MSVC to fail to compile invalid C++.)
Created attachment 820178 [details] [diff] [review]
fix v2: remove public function

[Thanks, Neil.]

Things seem to work (no build error & warning goes away) if I remove the public operator, as suggested in comment 8, so that seems to be the best way to resolve this.
Attachment #820008 - Attachment is obsolete: true
Attachment #820008 - Flags: review?(trev.saunders)
Attachment #820178 - Flags: review?(trev.saunders)
Flags: needinfo?(dholbert)
Attachment #820178 - Flags: review?(trev.saunders) → review+
https://hg.mozilla.org/integration/mozilla-inbound/rev/c6f3603c7216
Flags: in-testsuite-
https://hg.mozilla.org/mozilla-central/rev/c6f3603c7216
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla27
You need to log in before you can comment on or make changes to this bug.