Closed Bug 297146 Opened 15 years ago Closed 8 years ago

gcc warning in nsTString.h

Categories

(Core :: String, defect, trivial)

1.7 Branch
All
Linux
defect
Not set
trivial

Tracking

()

RESOLVED WORKSFORME

People

(Reporter: braden, Assigned: braden)

References

(Blocks 1 open bug)

Details

(Whiteboard: [build_warning])

Attachments

(1 file, 1 obsolete file)

1017 bytes, patch
darin.moz
: review-
Details | Diff | Splinter Review
gcc complains (via warning) that nsTAdoptingString_CharT's copy constructor does
not explicitly invoke the base class copy constructor.
Is this causing any problems?
Severity: normal → trivial
Attached patch Patch (obsolete) — Splinter Review
Assignee: nobody → braden
Status: NEW → ASSIGNED
Attachment #185738 - Flags: superreview?(darin)
Attachment #185738 - Flags: review?(darin)
Problems as in "causing incorrect code to be generated"? Not AFAIK.

Problems as in "causing gcc to emit a warning where it could be avoided with a
reasonable fix"? Yes... It's just an annoyance.
Comment on attachment 185738 [details] [diff] [review]
Patch

>Index: public/nsTString.h

>       nsTAdoptingString_CharT( const self_type& str )
>+        : nsTXPIDLString_CharT(str)
>         {
>           *this = str;
>         }

So, this seems to result in Assign(str) being called twice.  That
can't be right.
Attachment #185738 - Flags: superreview?(darin)
Attachment #185738 - Flags: review?(darin)
Attachment #185738 - Flags: review-
Um, yeah. Apparently I wasn't paying very close attention. Sorry. Revision coming.
Actually, the patch shouldn't introduce any problem that isn't already there. As
I understand it, the base class copy constructor is being called implicitly in
the current code.
Well then... it seems we found another bug ;-)
Attached patch PatchSplinter Review
I think it is sufficient simply to eliminate the explicit assignment to *this.
The effect of operator= seems only to be a call to Assign; which as you note is
called in a base class.
Attachment #185738 - Attachment is obsolete: true
Attachment #185920 - Flags: superreview?(darin)
Attachment #185920 - Flags: review?(darin)
Comment on attachment 185920 [details] [diff] [review]
Patch

Wait a second... this isn't correct either.  operator= is special in that it
mutates |str|.	This code changes seems to break that.
Attachment #185920 - Flags: superreview?(darin)
Attachment #185920 - Flags: review?(darin)
Attachment #185920 - Flags: review-
I don't understand why the original code was wrong.  It is calling the default
constructor for the base-class, which is what we want.  Then it calls operator=,
which transfers ownership of the string buffer.

I think the problem is that GCC is complaining about something benign.

I think this bug is INVALID.  We should find a way to instruct GCC to shut-up.
In that case, the right thing is probably to call the default ctor explicitly.
ok
Blocks: buildwarning
Whiteboard: [build_warning]
Could not find any warning in current logs of nightly build for linux and macos.

Closing this issue.
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → WORKSFORME
You need to log in before you can comment on or make changes to this bug.