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

Status: RESOLVED DUPLICATE of bug 718494

(Core :: DOM: Core & HTML, defect)

Description

[Mats Palmgren (inactive)]

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]

Comment 1

[Mats Palmgren (inactive)]

Attachment: fix

Comment 2

[Mats Palmgren (inactive)]

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!)

Comment 3

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

I'd rather you NS_ENSURE_SUCCESS'd right after the two assignments or drop the variable entirely.

Comment 4

[Mounir Lamouri (:mounir)]

(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.

Comment 5

[Mats Palmgren (inactive)]

Attachment: fix

Comment 8

[Mounir Lamouri (:mounir)]

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.

Comment 9

[Mats Palmgren (inactive)]

Attachment: 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.

Comment 11

[Mounir Lamouri (:mounir)]

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...

Comment 12

[Mats Palmgren (inactive)]

This was fixed by bug 718494.