nsHTMLTableElement.cpp and nsHTMLTableSectionElement.cpp does not compile with GCC 4.6 and --enable-warnings-as-errors

RESOLVED DUPLICATE of bug 718494

Status

()

RESOLVED DUPLICATE of bug 718494
7 years ago
7 years ago

People

(Reporter: mats, Assigned: mats)

Tracking

(Blocks: 1 bug)

Trunk
All
Linux
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 2 obsolete attachments)

(Assignee)

Description

7 years ago
c++ -o nsHTMLTableElement.o -c -I../../../../dist/stl_wrappers -I../../../../dist/system_wrappers -include config/gcc_hidden.h -DMOZILLA_INTERNAL_API -D_IMPL_NS_COM -DEXPORT_XPT_API -DEXPORT_XPTC_API -D_IMPL_NS_GFX -D_IMPL_NS_WIDGET -DIMPL_XREAPI -DIMPL_NS_NET -DIMPL_THEBES  -DSTATIC_EXPORTABLE_JS_API -DOSTYPE=\"Linux3.0\" -DOSARCH=Linux -DEXCLUDE_SKIA_DEPENDENCIES  -DOS_LINUX=1 -DOS_POSIX=1  -D_IMPL_NS_LAYOUT -Iipc/chromium/src -Iipc/glue -I../../../../ipc/ipdl/_ipdlheaders  -Icontent/html/content/src -I. -I../../../../dist/include -I../../../../dist/include/nsprpub  -I/OBJDIRdist/include/nspr -I/OBJDIRdist/include/nss     -Icontent/html/content/src/../../../base/src -Icontent/html/content/src/../../../events/src -Icontent/html/content/src/../../../xbl/src -Icontent/html/content/src/../../../xul/content/src -Icontent/html/content/src/../../../../layout/forms -Icontent/html/content/src/../../../../layout/style -Icontent/html/content/src/../../../../layout/tables -Icontent/html/content/src/../../../../layout/xul/base/src -Icontent/html/content/src/../../../../layout/generic -Icontent/html/content/src/../../../../dom/base -Icontent/html/content/src/../../../../editor/libeditor/base -Icontent/html/content/src/../../../../editor/libeditor/text -Icontent/html/content/src -Ixpcom/ds -Icontent/html/content/src/../../../../js/xpconnect/src   -fPIC -fno-rtti -Wall -Wpointer-arith -Woverloaded-virtual -Wsynth -Wno-ctor-dtor-privacy -Wno-non-virtual-dtor -Wcast-align -Wno-invalid-offsetof -Wno-variadic-macros -Werror=return-type -Wno-long-long -fno-exceptions -fno-strict-aliasing -std=gnu++0x -pthread -ffunction-sections -fdata-sections -pipe -DDEBUG -D_DEBUG -DTRACING -g -fno-omit-frame-pointer -Werror -Wno-error=uninitialized   -DMOZILLA_CLIENT -include ../../../../mozilla-config.h -MD -MF .deps/nsHTMLTableElement.pp content/html/content/src/nsHTMLTableElement.cpp
content/html/content/src/nsHTMLTableElement.cpp: In member function 'virtual nsresult nsHTMLTableElement::InsertRow(PRInt32, nsIDOMHTMLElement**)':
content/html/content/src/nsHTMLTableElement.cpp:709:12: error: variable 'rv' set but not used [-Werror=unused-but-set-variable]
(Assignee)

Comment 1

7 years ago
Created attachment 587329 [details] [diff] [review]
fix
Attachment #587329 - Flags: review?(mounir)
Attachment #587329 - Flags: review?(mounir) → review+
(Assignee)

Comment 2

7 years ago
Comment on attachment 587329 [details] [diff] [review]
fix

Sorry, I didn't think this through... now we might return an uninitialized
value.  (Hmm, I wonder why gcc didn't warn about that!)
Attachment #587329 - Attachment is obsolete: true
I'd rather you NS_ENSURE_SUCCESS'd right after the two assignments or drop the variable entirely.
(In reply to Ms2ger from comment #3)
> I'd rather you NS_ENSURE_SUCCESS'd right after the two assignments or drop
> the variable entirely.

I would prefer this actually.
(Assignee)

Comment 5

7 years ago
Created attachment 587426 [details] [diff] [review]
fix
Attachment #587426 - Flags: review?(mounir)
Duplicate of this bug: 717025
Duplicate of this bug: 717034
Comment on attachment 587426 [details] [diff] [review]
fix

Review of attachment 587426 [details] [diff] [review]:
-----------------------------------------------------------------

I would prefer a call to NS_ENSURE_SUCCESS to all calls returning a rv to reduce any possible unexpected issue.

r+ with that.

::: content/html/content/src/nsHTMLTableElement.cpp
@@ +742,5 @@
>          // insert the new row before the reference row we found above
>          rv = parent->InsertBefore(newRowNode, refRow,
>                                    getter_AddRefs(retChild));
>        }
> +      NS_ENSURE_SUCCESS(rv, rv);

I would prefer one call to NS_ENSURE_SUCCESS after each |rv = |.

::: content/html/content/src/nsHTMLTableSectionElement.cpp
@@ +194,3 @@
>    } else {
>      rv = AppendChild(rowNode, getter_AddRefs(retChild));
>    }

I would have preferred a NS_ENSURE_SUCCESS after each call here.
Attachment #587426 - Flags: review?(mounir) → review+
(Assignee)

Comment 9

7 years ago
Created attachment 587726 [details] [diff] [review]
fix

> I would prefer a call to NS_ENSURE_SUCCESS to all calls returning a rv to
> reduce any possible unexpected issue.

That adds considerable risk for regressions though, so please review
this carefully.

There's one call where I think the return value is intentionally
ignored;  I prefixed it with (void) to make it explicit.
Attachment #587726 - Flags: review?(mounir)
(Assignee)

Updated

7 years ago
Attachment #587426 - Attachment is obsolete: true
Summary: nsHTMLTableElement.cpp and nsHTMLTableSectionElement.cpp does not compile with --enable-warnings-as-errors → nsHTMLTableElement.cpp and nsHTMLTableSectionElement.cpp does not compile with GCC 4.6 and --enable-warnings-as-errors
Blocks: 187528
Hardware: x86_64 → All
Version: unspecified → Trunk
Duplicate of this bug: 718494
Comment on attachment 587726 [details] [diff] [review]
fix

Review of attachment 587726 [details] [diff] [review]:
-----------------------------------------------------------------

r=me but I'm not sure you need all those NS_ENSURE_SUCCESS. I was expecting to see them after where |rv| was already used...
Attachment #587726 - Flags: review?(mounir) → review+
(Assignee)

Comment 12

7 years ago
This was fixed by bug 718494.
Status: NEW → RESOLVED
Last Resolved: 7 years ago
Resolution: --- → DUPLICATE
Duplicate of bug: 718494
You need to log in before you can comment on or make changes to this bug.