Closed Bug 792090 Opened 7 years ago Closed 7 years ago

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

Categories

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

x86_64
Linux
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla19

People

(Reporter: dholbert, Assigned: mccr8)

References

(Blocks 1 open bug)

Details

Attachments

(1 file, 1 obsolete file)

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]
}
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.
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.
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.)
(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.)
Blocks: 791345
This is actually from bug 788532.  Which started exercising the code from bug 768692.
Blocks: 788532, 768692
No longer blocks: 791345
(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.
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
}
...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.
> Last I heard,  "(void)result;" is the recommended way, at least in GCC.

That's what is done in the cycle collector macros.
I could work on putting in a |(void)result;| thing if this bug is low-priority and probably not very hard.
Assignee: nobody → continuation
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.
Attached patch ignore unused error value (obsolete) — Splinter Review
The supportsNamedProperties() case doesn't have the same problem, so we don't have to do anything there.
Attachment #680150 - Flags: review?(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.
Attachment #680150 - Flags: review?(bzbarsky) → review+
(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.
Carrying forward bz's r+.
Attachment #680150 - Attachment is obsolete: true
Attachment #680799 - Flags: review+
https://hg.mozilla.org/mozilla-central/rev/e997348bdeaa
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla19
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.