Re: A reminder about MOZ_MUST_USE and [must_use]

[ISHIKAWA,chiaki]

On 2017/01/20 8:10, Nicholas Nethercote wrote:

There are lots
of functions where not checking the return value is reasonable, such as
close().


A file opened for writing and is buffered will flush pending data to 
disk upon Close() and may encounter the error such as disk full AT THAT 
POINT, and so the return value of

Close() MUST BE CHECKED for those cases.
[I am fighting to fix the issue since C-C TB fails to use buffered write 
which causes so much I/O overhead, but then I found the return value of 
Close() are not checked at all. Ugh... So I have to fix C-C TB to make 
it check the return value of Close() and take appropriate error 
reporting/recovery action before I can enable buffering. Very 
disappointing...]


A blanket statement above can put bad ideas in many people's mind :-(

Of course, if the "close()" above refers to POSIX close(), fine.

TIA


___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform

Re: A reminder about MOZ_MUST_USE and [must_use]

[Michael Layzell]

It wouldn't be too hard to automatically generate Result
wrapper methods automatically for all of our XPIDL interfaces. I had a
prototype branch at one point which did this on the rust side, as part of
my now-dead rust XPIDL bindings.

That would convert a good number of our fallable calls to using Result. We might even be able to look into doing something similar with
our WebIDL bindings and ErrorResult.

On Fri, Jan 20, 2017 at 8:59 AM, Ted Mielczarek  wrote:

> On Fri, Jan 20, 2017, at 08:19 AM, Nicolas B. Pierron wrote:
> > > The Rust case is helped by the fact that `Result` is the defacto type
> > > for returning success or error, and it's effectively `must_use`. We
> > > don't have a similar default convention in C++.
> >
> > We have
> >
> > http://searchfox.org/mozilla-central/rev/30fcf167af036aeddf322de44a2fad
> d370acfd2f/mfbt/Result.h#173
> >
> > we just have to make it the default convention now.
>
> Yes, and this is great, I just meant that in Rust 99% of code that's
> returning success/failure is using `Result` because it's in the standard
> library, whereas in C++ there's not an equivalent. `mozilla::Result` is
> great and I hope we can convert lots of Gecko code to use it, but we
> have *a lot* of Gecko code that's not already there.
>
> -Ted
> ___
> dev-platform mailing list
> dev-platform@lists.mozilla.org
> https://lists.mozilla.org/listinfo/dev-platform
>
___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform

Re: A reminder about MOZ_MUST_USE and [must_use]

[Ted Mielczarek]

On Fri, Jan 20, 2017, at 08:19 AM, Nicolas B. Pierron wrote:
> > The Rust case is helped by the fact that `Result` is the defacto type
> > for returning success or error, and it's effectively `must_use`. We
> > don't have a similar default convention in C++.
> 
> We have
> 
> http://searchfox.org/mozilla-central/rev/30fcf167af036aeddf322de44a2fadd370acfd2f/mfbt/Result.h#173
> 
> we just have to make it the default convention now.

Yes, and this is great, I just meant that in Rust 99% of code that's
returning success/failure is using `Result` because it's in the standard
library, whereas in C++ there's not an equivalent. `mozilla::Result` is
great and I hope we can convert lots of Gecko code to use it, but we
have *a lot* of Gecko code that's not already there.

-Ted
___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform

Re: A reminder about MOZ_MUST_USE and [must_use]

[Nicolas B. Pierron]

On 01/20/2017 12:00 PM, Ted Mielczarek wrote:

On Thu, Jan 19, 2017, at 07:00 PM, gsquel...@mozilla.com wrote:

I think the point is that it's not obvious that "must check the return
value" is a sufficiently-dominant common case for arbitrary return values.
FWIW, Rust took the [must_use] rather than [can_ignore] approach too.


That's unfortunate. But real-world data must trump my idealism in the
end. :-)


The Rust case is helped by the fact that `Result` is the defacto type
for returning success or error, and it's effectively `must_use`. We
don't have a similar default convention in C++.


We have

http://searchfox.org/mozilla-central/rev/30fcf167af036aeddf322de44a2fadd370acfd2f/mfbt/Result.h#173

we just have to make it the default convention now.

--
Nicolas B. Pierron
___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform

Re: A reminder about MOZ_MUST_USE and [must_use]

[Ted Mielczarek]

On Thu, Jan 19, 2017, at 07:00 PM, gsquel...@mozilla.com wrote:
> > I think the point is that it's not obvious that "must check the return
> > value" is a sufficiently-dominant common case for arbitrary return values.
> > FWIW, Rust took the [must_use] rather than [can_ignore] approach too.
> 
> That's unfortunate. But real-world data must trump my idealism in the
> end. :-)

The Rust case is helped by the fact that `Result` is the defacto type
for returning success or error, and it's effectively `must_use`. We
don't have a similar default convention in C++.

-Ted
___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform

Re: A reminder about MOZ_MUST_USE and [must_use]

[gsquelart]

On Friday, January 20, 2017 at 10:36:46 AM UTC+11, Bobby Holley wrote:
> On Thu, Jan 19, 2017 at 3:26 PM,  wrote:
> 
> > On Friday, January 20, 2017 at 10:13:54 AM UTC+11, Nicholas Nethercote
> > wrote:
> > > On Fri, Jan 20, 2017 at 10:01 AM,  wrote:
> > >
> > > > And the next step would be to make must-use the default, and have
> > > > MOZ_CAN_IGNORE for the rest. ;-)
> > > >
> > >
> > > I actually tried this with all XPIDL methods. After adding several
> > hundred
> > > "Unused <<" annotations for calls that legitimately didn't need to check
> > > the return value -- and I was only a fraction of the way through the
> > > codebase -- I decided that a big bang approach wasn't going to work. So I
> > > then implemented [must_use] as an incremental alternative.
> > >
> > > Nick
> >
> > I guess my slightly-tongue-in-cheek suggestion was to reverse MOZ_MUST_USE.
> >
> 
> I think the point is that it's not obvious that "must check the return
> value" is a sufficiently-dominant common case for arbitrary return values.
> FWIW, Rust took the [must_use] rather than [can_ignore] approach too.

That's unfortunate. But real-world data must trump my idealism in the end. :-)


> It probably depends a lot on the return value type.

Makes sense.


Another idea:
Could all non-void const methods (those that don't modify *this) be 
MOZ_MUST_USE by default? They supposedly don't have side-effects, so why would 
their return value ever be ignored?


> > So in cases where a return value from a function can be ignored, instead
> > of adding "Unused <<" at all the call sites, you'd add MOZ_CAN_IGNORE in
> > front of the function declaration. Or possibly MOZ_CAN_IGNORE_TYPE if more
> > appropriate.
> >
> > Of course I'm sure it would still take a lot of work!
> >
> > Maybe there could be a slow hybrid approach:
> > - Start with Eric's suggestion to make a directory MOZ_MUST_USE'able.
> > - Add "Unused <<" for isolated failures, or add MOZ_CAN_IGNORE[_TYPE] for
> > larger failures.
> > - Gradually "infect" more directories.
> > - Once it's everywhere, make MOZ_MUST_USE the default, and remove the
> > above scaffolding.
> > - Profit!
> >
> > Gerald
> > ___
> > dev-platform mailing list
> > dev-pl...@lists.mozilla.org
> > https://lists.mozilla.org/listinfo/dev-platform
> >

___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform

Re: A reminder about MOZ_MUST_USE and [must_use]

[Bobby Holley]

On Thu, Jan 19, 2017 at 3:26 PM,  wrote:

> On Friday, January 20, 2017 at 10:13:54 AM UTC+11, Nicholas Nethercote
> wrote:
> > On Fri, Jan 20, 2017 at 10:01 AM,  wrote:
> >
> > > And the next step would be to make must-use the default, and have
> > > MOZ_CAN_IGNORE for the rest. ;-)
> > >
> >
> > I actually tried this with all XPIDL methods. After adding several
> hundred
> > "Unused <<" annotations for calls that legitimately didn't need to check
> > the return value -- and I was only a fraction of the way through the
> > codebase -- I decided that a big bang approach wasn't going to work. So I
> > then implemented [must_use] as an incremental alternative.
> >
> > Nick
>
> I guess my slightly-tongue-in-cheek suggestion was to reverse MOZ_MUST_USE.
>

I think the point is that it's not obvious that "must check the return
value" is a sufficiently-dominant common case for arbitrary return values.
FWIW, Rust took the [must_use] rather than [can_ignore] approach too.

It probably depends a lot on the return value type.


> So in cases where a return value from a function can be ignored, instead
> of adding "Unused <<" at all the call sites, you'd add MOZ_CAN_IGNORE in
> front of the function declaration. Or possibly MOZ_CAN_IGNORE_TYPE if more
> appropriate.
>
> Of course I'm sure it would still take a lot of work!
>
> Maybe there could be a slow hybrid approach:
> - Start with Eric's suggestion to make a directory MOZ_MUST_USE'able.
> - Add "Unused <<" for isolated failures, or add MOZ_CAN_IGNORE[_TYPE] for
> larger failures.
> - Gradually "infect" more directories.
> - Once it's everywhere, make MOZ_MUST_USE the default, and remove the
> above scaffolding.
> - Profit!
>
> Gerald
> ___
> dev-platform mailing list
> dev-platform@lists.mozilla.org
> https://lists.mozilla.org/listinfo/dev-platform
>
___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform

Re: A reminder about MOZ_MUST_USE and [must_use]

[Chris Peterson]

On 1/19/2017 3:13 PM, Nicholas Nethercote wrote:

On Fri, Jan 20, 2017 at 10:01 AM,  wrote:


> And the next step would be to make must-use the default, and have
> MOZ_CAN_IGNORE for the rest. ;-)
>

I actually tried this with all XPIDL methods. After adding several hundred
"Unused <<" annotations for calls that legitimately didn't need to check
the return value -- and I was only a fraction of the way through the
codebase -- I decided that a big bang approach wasn't going to work. So I
then implemented [must_use] as an incremental alternative.


To encourage all new XPIDL methods to use must_use, could you add 
MOZ_MUST_USE to the C++ macro definition of NS_IMETHOD and rename all 
the existing uses of NS_IMETHOD to something like 
NS_IMETHOD_RESULT_OPTIONAL or NS_IMETHOD_RESULT_UNUSED? Developers 
adding new methods will reach the familiar (and shorter) NS_IMETHOD 
macro and get must_use for free.

___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform

Re: A reminder about MOZ_MUST_USE and [must_use]

[gsquelart]

On Friday, January 20, 2017 at 10:13:54 AM UTC+11, Nicholas Nethercote wrote:
> On Fri, Jan 20, 2017 at 10:01 AM,  wrote:
> 
> > And the next step would be to make must-use the default, and have
> > MOZ_CAN_IGNORE for the rest. ;-)
> >
> 
> I actually tried this with all XPIDL methods. After adding several hundred
> "Unused <<" annotations for calls that legitimately didn't need to check
> the return value -- and I was only a fraction of the way through the
> codebase -- I decided that a big bang approach wasn't going to work. So I
> then implemented [must_use] as an incremental alternative.
> 
> Nick

I guess my slightly-tongue-in-cheek suggestion was to reverse MOZ_MUST_USE.

So in cases where a return value from a function can be ignored, instead of 
adding "Unused <<" at all the call sites, you'd add MOZ_CAN_IGNORE in front of 
the function declaration. Or possibly MOZ_CAN_IGNORE_TYPE if more appropriate.

Of course I'm sure it would still take a lot of work!

Maybe there could be a slow hybrid approach:
- Start with Eric's suggestion to make a directory MOZ_MUST_USE'able.
- Add "Unused <<" for isolated failures, or add MOZ_CAN_IGNORE[_TYPE] for 
larger failures.
- Gradually "infect" more directories.
- Once it's everywhere, make MOZ_MUST_USE the default, and remove the above 
scaffolding.
- Profit!

Gerald
___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform

Re: A reminder about MOZ_MUST_USE and [must_use]

[Nicholas Nethercote]

On Fri, Jan 20, 2017 at 9:29 AM, Eric Rescorla  wrote:

> What would actually be very helpful would be a way to selectively turn on
> checking of
> *all* return values in a given file/subdirectory. Is there some
> mechanism/plan for that?
>

Not that I know of, other than manually annotating them.

As someone who's spent some time adding these annotations, I think this
idea sounds great in theory but turns out to be impractical. There are lots
of functions where not checking the return value is reasonable, such as
close(). And there are lots of functions that have a return value due to
interface constraints, but it doesn't need to be checked, such as many
XPIDL methods. And yes, you can use mozilla::Unused to mark functions that
don't need checking, but that gets annoying quickly.

If you want to be ambitious and avoid piecemeal approaches, I suggest using
the new mozilla::Result type, which has the MOZ_MUST_USE_TYPE annotation on
it that means static analysis builds will fail if results aren't checked.
Doing this stuff in new code is easier than retrofitting old code.

Nick
___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform

Re: A reminder about MOZ_MUST_USE and [must_use]

[gsquelart]

And the next step would be to make must-use the default, and have 
MOZ_CAN_IGNORE for the rest. ;-)

Gerald (who is not volunteering!)

On Friday, January 20, 2017 at 9:30:13 AM UTC+11, Eric Rescorla wrote:
> What would actually be very helpful would be a way to selectively turn on
> checking of
> *all* return values in a given file/subdirectory. Is there some
> mechanism/plan for that?
> 
> Thanks,
> -Ekr
> 
> 
> On Thu, Jan 19, 2017 at 2:09 PM, Nicholas Nethercote  > wrote:
> 
> > Hi,
> >
> > We have two annotations that can be used to prevent missing return value
> > checks:
> >
> > - MOZ_MUST_USE for C++ functions where the return type indicates
> > success/failure, e.g. nsresult, bool (in some instances), and some other
> > types.
> >
> > - [must_use] for IDL methods and properties where the nsresult value should
> > be checked.
> >
> > We have *many* functions/methods/properties for which these annotations are
> > appropriate, and *many* missing return value checks. Unfortunately, trying
> > to fix these proactively is a frustrating and thankless task, because it's
> > difficult to know in advance which missing checks are likely to cause
> > problems in advance, and adding missing checks is not always
> > straightforward.
> >
> > However, if you do see clearly buggy behaviour (e.g. a crash) caused by a
> > missing return value, please take the opportunity to retroactively add the
> > annotation(s) in that case!
> > https://bugzilla.mozilla.org/show_bug.cgi?id=1331619 is a good example of
> > such a bug, and https://bugzilla.mozilla.org/show_bug.cgi?id=1332453 is
> > the
> > follow-up to add the annotations.
> >
> > Nick
> > ___
> > dev-platform mailing list
> > dev-pl...lists.mozilla.org
> > https://lists.mozilla.org/listinfo/dev-platform
> >

___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform

Re: A reminder about MOZ_MUST_USE and [must_use]

[Eric Rescorla]

What would actually be very helpful would be a way to selectively turn on
checking of
*all* return values in a given file/subdirectory. Is there some
mechanism/plan for that?

Thanks,
-Ekr


On Thu, Jan 19, 2017 at 2:09 PM, Nicholas Nethercote  wrote:

> Hi,
>
> We have two annotations that can be used to prevent missing return value
> checks:
>
> - MOZ_MUST_USE for C++ functions where the return type indicates
> success/failure, e.g. nsresult, bool (in some instances), and some other
> types.
>
> - [must_use] for IDL methods and properties where the nsresult value should
> be checked.
>
> We have *many* functions/methods/properties for which these annotations are
> appropriate, and *many* missing return value checks. Unfortunately, trying
> to fix these proactively is a frustrating and thankless task, because it's
> difficult to know in advance which missing checks are likely to cause
> problems in advance, and adding missing checks is not always
> straightforward.
>
> However, if you do see clearly buggy behaviour (e.g. a crash) caused by a
> missing return value, please take the opportunity to retroactively add the
> annotation(s) in that case!
> https://bugzilla.mozilla.org/show_bug.cgi?id=1331619 is a good example of
> such a bug, and https://bugzilla.mozilla.org/show_bug.cgi?id=1332453 is
> the
> follow-up to add the annotations.
>
> Nick
> ___
> dev-platform mailing list
> dev-platform@lists.mozilla.org
> https://lists.mozilla.org/listinfo/dev-platform
>
___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform

A reminder about MOZ_MUST_USE and [must_use]

[Nicholas Nethercote]

Hi,

We have two annotations that can be used to prevent missing return value
checks:

- MOZ_MUST_USE for C++ functions where the return type indicates
success/failure, e.g. nsresult, bool (in some instances), and some other
types.

- [must_use] for IDL methods and properties where the nsresult value should
be checked.

We have *many* functions/methods/properties for which these annotations are
appropriate, and *many* missing return value checks. Unfortunately, trying
to fix these proactively is a frustrating and thankless task, because it's
difficult to know in advance which missing checks are likely to cause
problems in advance, and adding missing checks is not always
straightforward.

However, if you do see clearly buggy behaviour (e.g. a crash) caused by a
missing return value, please take the opportunity to retroactively add the
annotation(s) in that case!
https://bugzilla.mozilla.org/show_bug.cgi?id=1331619 is a good example of
such a bug, and https://bugzilla.mozilla.org/show_bug.cgi?id=1332453 is the
follow-up to add the annotations.

Nick
___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform