792090 - ClientRectListBinding.cpp:381:23: error: variable ‘result’ set but not used [-Werror=unused-but-set-variable]

Status: RESOLVED FIXED

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

Description

[Daniel Holbert [:dholbert]]

New GCC 4.7 build warning, in my build this morning (treated as an error in --enable-warnings-as-errors builds):
{
ClientRectListBinding.cpp: In member function ‘virtual bool mozilla::dom::ClientRectListBinding::DOMProxyHandler::hasOwn(JSContext*, JSObject*, jsid, bool*)’:
ClientRectListBinding.cpp:381:23: error: variable ‘result’ set but not used [-Werror=unused-but-set-variable]
}

Comment 1

[Daniel Holbert [:dholbert]]

This is for the file $OBJDIR/dom/bindings/ClientRectListBinding.cpp , which seems to be a generated file.

The code in question from that file is:
> bool
> DOMProxyHandler::hasOwn(JSContext* cx, JSObject* proxy, jsid id, bool* bp)
> {
>   int32_t index = GetArrayIndexFromId(cx, id);
>   if (index >= 0) {
>     nsClientRectList* self = UnwrapProxy(proxy);
>     bool found;
>     nsIDOMClientRect* result;
>     result = self->IndexedGetter(index, found);
> 
>     *bp = found;
>     return true;
>   }

Note that "result" is unused there -- that's what's triggering a warning.

Comment 2

[Daniel Holbert [:dholbert]]

Also: The dom/bindings directory has been marked as FAIL_ON_WARNINGS for over a month:
  https://hg.mozilla.org/mozilla-central/diff/1bbc0b65dffb/dom/bindings/Makefile.in
So, this build-failure is from a new warning, rather than (as sometimes occurs) being from a new FAIL_ON_WARNINGS annotation.

Comment 3

[Boris Zbarsky [:bzbarsky]]

We should probably mark "result" as unused here, yes.  How do we annotate that?

(The point is, we don't care about the return value at all here; we just want the "found" outparam.)

Comment 4

[Daniel Holbert [:dholbert]]

(Based on hg pushlog for /dom/bindings directory, I'm assuming this is from bug 791345, the most substantial recent change.  Marking dependency -- feel free to update if I'm mis-blaming.)

(In reply to Boris Zbarsky (:bz) from comment #3)
> We should probably mark "result" as unused here, yes.  How do we annotate
> that?

Last I heard,  "(void)result;" is the recommended way, at least in GCC.

(jdm/Waldo claim that's not enough for some compilers (bug 711799 comment 8), but I don't think it's worth adding a hacky "->Use()" method just to make those other compilers happy in this case.)

Comment 5

[Boris Zbarsky [:bzbarsky]]

This is actually from bug 788532.  Which started exercising the code from bug 768692.

Comment 6

[Daniel Holbert [:dholbert]]

(In reply to Boris Zbarsky (:bz) from comment #5)
> This is actually from bug 788532.

erm, yeah -- that's the bug I meant to tag.

Comment 8

[Daniel Holbert [:dholbert]]

I also encounter this build warning for "HTMLCollectionBinding.cpp", FWIW:
{
Warning: -Wunused-but-set-variable in  $OBJDIR/dom/bindings/HTMLCollectionBinding.cpp: variable ‘result’ set but not used
}

Comment 9

[Daniel Holbert [:dholbert]]

...though it looks like the relevant FAIL_ON_WARNINGS annotation was removed as part of this large patch: https://hg.mozilla.org/mozilla-central/rev/72e482dbd384#l46.12

...so while these build warnings are still there, they shouldn't be fatal anymore, at least.

Comment 10

[Andrew McCreight [:mccr8]]

> Last I heard,  "(void)result;" is the recommended way, at least in GCC.

That's what is done in the cycle collector macros.

Comment 11

[Andrew McCreight [:mccr8]]

I could work on putting in a |(void)result;| thing if this bug is low-priority and probably not very hard.

Comment 12

[Boris Zbarsky [:bzbarsky]]

Go for it.

Comment 13

[Andrew McCreight [:mccr8]]

It looks like if I could pass along some kind of indicator that the return value can be ignored, that would work, too, but I guess complicating the code gen to make the resulting code a tiny bit prettier is probably not worthwhile.

Comment 14

[Andrew McCreight [:mccr8]]

Attachment: ignore unused error value

The supportsNamedProperties() case doesn't have the same problem, so we don't have to do anything there.

Comment 15

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 680150 [details] [diff] [review]
ignore unused error value

Er.... why doesn't the CGProxyNamedPresenceChecker have the same problem?  I'm seeing generated code like this:

    JSObject* result;
    result = self->NamedGetter(cx, name, found, rv);
    if (rv.Failed()) {
      return ThrowMethodFailedWithDetails<true>(cx, rv, "HTMLCollection", "namedItem");
    }
    *bp = found;
    return true;

which seems like it should have the same problem.

I'd rather the "(void)result;" go in the two presence checker classes, not all their consumers, though.  r=me with that changed.

Comment 16

[Andrew McCreight [:mccr8]]

(In reply to Boris Zbarsky (:bz) from comment #15)
> Comment on attachment 680150 [details] [diff] [review]
> ignore unused error value
> 
> Er.... why doesn't the CGProxyNamedPresenceChecker have the same problem? 

Ah, the only case I checked was DOMStringMapBinding, which generates code like this:
    nsString result;
    self->NamedGetter(name, found, result);

The result isn't returned, so it doesn't matter that it is ignored. The other 3 instances of it just return it, so they are in fact going to generate warnings, like you said.  Oops.

> I'd rather the "(void)result;" go in the two presence checker classes, not
> all their consumers, though.  r=me with that changed.

Good idea.

Comment 17

[Andrew McCreight [:mccr8]]

Attachment: updated to address review comments

Carrying forward bz's r+.

Comment 18

[Andrew McCreight [:mccr8]]

Linux64 try run looked fine.
https://hg.mozilla.org/integration/mozilla-inbound/rev/e997348bdeaa

Comment 19

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/e997348bdeaa