801464 - warn if the result of NS_{FAILED,SUCCEEDED}() is not used

Status: NEW

(Core :: XPCOM, defect)

Description

[Trevor Saunders (:tbsaunde)]

It doesn't make any sense to call these functions and not use the result.

Comment 1

[Trevor Saunders (:tbsaunde)]

Attachment: patch

Comment 2

[Justin Lebar (not reading bugmail)]

Comment on attachment 671267 [details] [diff] [review]
patch

sgtm, but you'll need a try run here to see if warnings-as-errors causes this to burn the build.

Comment 3

[Mats Palmgren (inactive)]

I'm afraid I've bit-rotted your patch with bug 802473, sorry.
I don't see a way to have MOZ_WARN_UNUSED_RESULT after that
change, since the result is always given to NS_(UN)LIKELY,
that is __builtin_expect().  One possibility would be to move
NS_(UN)LIKELY inside the function instead, but I read somewhere
that compilers can't propagate branch prediction through function
calls (I don't know if this is true though).

Comment 4

[Trevor Saunders (:tbsaunde)]

(In reply to Mats Palmgren [:mats] from comment #3)
> I'm afraid I've bit-rotted your patch with bug 802473, sorry.

that's ok.

> I don't see a way to have MOZ_WARN_UNUSED_RESULT after that
> change, since the result is always given to NS_(UN)LIKELY,
> that is __builtin_expect().  One possibility would be to move
> NS_(UN)LIKELY inside the function instead, but I read somewhere
> that compilers can't propagate branch prediction through function
> calls (I don't know if this is true though).

I could believe that about not lto compilers with non inline functions, but I'd sort of expect and really hope they can optimize across an inlined function.

One thing I think we could certainly do is to make NS_{FAILED,SUCCEEDED}() be functions only on debug builds where we don't care about perf, but we'd get most of the same benefit.  That would actually have the side benefit that the compiler wouldn't warn about fairly reasonable things like MOZ_ALWAYS_TRUE(NS_SUCCEEDED(blah)) which atleast some editor code does.