Re: Error handling with MOZ_MUST_USE

2016-11-14 Thread Wei-Cheng Pan 
Hi,

I've updated the coding style document, added something about
MOZ_MUST_USE and MOZ_MUST_USE_TYPE on MDN:

https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Coding_Style#Error_handling

Best,
Wei-Cheng Pan


On 04/11/2016 12:39 AM, Michael Layzell wrote:
> We also have a static analysis attribute MOZ_MUST_USE_TYPE which can
> be added to class declarations to ensure that those declarations are
> always used when they are returned. We currently are using it to
> annotate already_AddRefed values. If there are other types which
> universally should be checked when used as a return value, that
> annotation may be a useful shortcut.
>
> On Thu, Nov 3, 2016 at 3:42 AM, Wei-Cheng Pan  > wrote:
>
> Hi,
>
> As :njn announced before [1], we are trying to add MOZ_MUST_USE to all
> fail-able functions, which is tracking in bug 1268766 [2].
>
> We have bugs to track the progress for each directory.
> If anyone is interested, please feel free to take any of the bugs.
>
> We hope all reviewers also mind the following rules, so that new code
> may benefit by the static checking.
> I'll update the style guide to reflect these rules later.
>
> The attribute is meant to avoid possible fatal failure.
> All failure should be handled, reported, asserted, or at least
> caught by
> Unused (and better leave a comment, see bug 1301574 [3]).
>
> You can find the rule proposed by :njn in the bug comment [4].
> Because now we don't need to worry about NS_IMETHOD (see bug 1295825
> [5]), we have updated rules:
>
> 1. search all *.h and *.idl
> 2. add [must_use] for *.idl
> 3. add MOZ_MUST_USE for all functions that return |nsresult| or |bool|
> in *.h
>
> Also with some exceptions:
>
> 1. Predicates, getters which return |bool|.
>
> 2. IPC method implementation (e.g. bool RecvSomeMessage()).
>
> 3. Most callers just check the output parameter. e.g.:
>
> nsresult
> SomeMap::GetValue(const nsString& key, nsString& value);
>
> nsString value;
> rv = map->GetValue(key, value);
>
> If it failed, of course |value| will be empty.
> But if it succeed, |value| may still be empty because the stored value
> is empty in the first place.
> So just check |value| is a common idiom.
> This was also discussed in thread [1], and seems we still have no
> conclusion.
>
> Such grey zone will depend on each reviewers' own judgement.
>
> Wei-Cheng Pan
>
> [1]
> 
> https://groups.google.com/forum/#!msg/mozilla.dev.platform/dS5Dz6SfO3w/avzIxb7CBAAJ
> 
> 
> [2] https://bugzilla.mozilla.org/show_bug.cgi?id=MOZ_MUST_USE
> 
> [3] https://bugzilla.mozilla.org/show_bug.cgi?id=1301574
> 
> [4] https://bugzilla.mozilla.org/show_bug.cgi?id=1268766#c5
> 
> [5] https://bugzilla.mozilla.org/show_bug.cgi?id=1295825
> 
>
> ___
> 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: Error handling with MOZ_MUST_USE

2016-11-03 Thread Michael Layzell 
We also have a static analysis attribute MOZ_MUST_USE_TYPE which can be
added to class declarations to ensure that those declarations are always
used when they are returned. We currently are using it to annotate
already_AddRefed values. If there are other types which universally should
be checked when used as a return value, that annotation may be a useful
shortcut.

On Thu, Nov 3, 2016 at 3:42 AM, Wei-Cheng Pan  wrote:

> Hi,
>
> As :njn announced before [1], we are trying to add MOZ_MUST_USE to all
> fail-able functions, which is tracking in bug 1268766 [2].
>
> We have bugs to track the progress for each directory.
> If anyone is interested, please feel free to take any of the bugs.
>
> We hope all reviewers also mind the following rules, so that new code
> may benefit by the static checking.
> I'll update the style guide to reflect these rules later.
>
> The attribute is meant to avoid possible fatal failure.
> All failure should be handled, reported, asserted, or at least caught by
> Unused (and better leave a comment, see bug 1301574 [3]).
>
> You can find the rule proposed by :njn in the bug comment [4].
> Because now we don't need to worry about NS_IMETHOD (see bug 1295825
> [5]), we have updated rules:
>
> 1. search all *.h and *.idl
> 2. add [must_use] for *.idl
> 3. add MOZ_MUST_USE for all functions that return |nsresult| or |bool|
> in *.h
>
> Also with some exceptions:
>
> 1. Predicates, getters which return |bool|.
>
> 2. IPC method implementation (e.g. bool RecvSomeMessage()).
>
> 3. Most callers just check the output parameter. e.g.:
>
> nsresult
> SomeMap::GetValue(const nsString& key, nsString& value);
>
> nsString value;
> rv = map->GetValue(key, value);
>
> If it failed, of course |value| will be empty.
> But if it succeed, |value| may still be empty because the stored value
> is empty in the first place.
> So just check |value| is a common idiom.
> This was also discussed in thread [1], and seems we still have no
> conclusion.
>
> Such grey zone will depend on each reviewers' own judgement.
>
> Wei-Cheng Pan
>
> [1]
> https://groups.google.com/forum/#!msg/mozilla.dev.platform/dS5Dz6SfO3w/
> avzIxb7CBAAJ
> [2] https://bugzilla.mozilla.org/show_bug.cgi?id=MOZ_MUST_USE
> [3] https://bugzilla.mozilla.org/show_bug.cgi?id=1301574
> [4] https://bugzilla.mozilla.org/show_bug.cgi?id=1268766#c5
> [5] https://bugzilla.mozilla.org/show_bug.cgi?id=1295825
>
> ___
> 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

Error handling with MOZ_MUST_USE

2016-11-03 Thread Wei-Cheng Pan 
Hi,

As :njn announced before [1], we are trying to add MOZ_MUST_USE to all
fail-able functions, which is tracking in bug 1268766 [2].

We have bugs to track the progress for each directory.
If anyone is interested, please feel free to take any of the bugs.

We hope all reviewers also mind the following rules, so that new code
may benefit by the static checking.
I'll update the style guide to reflect these rules later.

The attribute is meant to avoid possible fatal failure.
All failure should be handled, reported, asserted, or at least caught by
Unused (and better leave a comment, see bug 1301574 [3]).

You can find the rule proposed by :njn in the bug comment [4].
Because now we don't need to worry about NS_IMETHOD (see bug 1295825
[5]), we have updated rules:

1. search all *.h and *.idl
2. add [must_use] for *.idl
3. add MOZ_MUST_USE for all functions that return |nsresult| or |bool|
in *.h

Also with some exceptions:

1. Predicates, getters which return |bool|.

2. IPC method implementation (e.g. bool RecvSomeMessage()).

3. Most callers just check the output parameter. e.g.:

nsresult
SomeMap::GetValue(const nsString& key, nsString& value);

nsString value;
rv = map->GetValue(key, value);

If it failed, of course |value| will be empty.
But if it succeed, |value| may still be empty because the stored value
is empty in the first place.
So just check |value| is a common idiom.
This was also discussed in thread [1], and seems we still have no
conclusion.

Such grey zone will depend on each reviewers' own judgement.

Wei-Cheng Pan

[1]
https://groups.google.com/forum/#!msg/mozilla.dev.platform/dS5Dz6SfO3w/avzIxb7CBAAJ
[2] https://bugzilla.mozilla.org/show_bug.cgi?id=MOZ_MUST_USE
[3] https://bugzilla.mozilla.org/show_bug.cgi?id=1301574
[4] https://bugzilla.mozilla.org/show_bug.cgi?id=1268766#c5
[5] https://bugzilla.mozilla.org/show_bug.cgi?id=1295825

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