Re: A reminder about MOZ_MUST_USE and [must_use]
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]
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]
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]
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]
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]
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]
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]
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]
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]
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 ___ 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]
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]
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]
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]
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