02.04.2020 18:06, Markus Armbruster wrote:
Vladimir Sementsov-Ogievskiy <vsement...@virtuozzo.com> writes:
02.04.2020 11:55, Markus Armbruster wrote:
Peter Maydell <peter.mayd...@linaro.org> writes:
On Thu, 2 Apr 2020 at 07:11, Vladimir Sementsov-Ogievskiy
<vsement...@virtuozzo.com> wrote:
Somehow, in general, especially with long function names and long parameter
lists I prefer
ret = func(..);
if (ret < 0) {
return ret;
}
Personally I prefer the other approach -- this one has an extra line
in the source and
needs an extra local variable.
Me too, except when func(...) is so long that
if (func(...) < 0) {
becomes illegible like
if (func(... yadda, yadda,
yadda, ...) < 0) {
Then an extra variable can improve things.
Are you sure that adding a lot of boolean functions is a good idea? I somehow
feel better with more usual int functions with -errno on failure.
Bool is a good return value for functions which are boolean by nature: checks,
is something correspond to some criteria. But for reporting an error I'd prefer
-errno.
When would we want to return an errno? I thought the whole point of the
Error* was that that was where information about the error was returned.
If all your callsites are just going to do "if (ret < 0) { ... } then having
the functions pick an errno value to return is just extra work.
0/-1 vs. true/false is a matter of convention. Lacking convention, it's
a matter of taste. >
0/-1 vs. 0/-errno depends on the function and its callers. When -errno
enables callers to distinguish failures in a sane and simple way, use
it. When -errno feels "natural", I'd say feel free to use it even when
all existing callers only check < 0.
When you return non-null/null or true/false on success/error, neglecting
to document that in a function contract can perhaps be tolerated; you're
using the return type the common, obvious way. But when you return 0/-1
or 0/-errno, please spell it out. I've seen too many "Operation not
permitted" that were actually -1 mistaken for -EPERM. Also too many
functions that return -1 for some failures and -errno for others.
I just want to add one note:
OK, you like the pattern
if (func()) {
<handle error>
}
, I can live with it.
I believe, we have a lot of such patterns already in code.
Now, we are going to add a lot of functions, returning true on success and
false on failure, so add a lot of patterns
if (!func()) {
<handle error>
}
We already have this pattern all over the place with functions returning
non-null pointers on success, null pointer on failure.
Functions returning pointer are simpler to distinguish by name.
Hmm, strange. But this pattern lose the pointer.. You mean
ptr = func();
if (!ptr) {
<handle error>
}
this is much more understandable. Usually ptr variable name and function name -
all will help to understand that it's all about pointer.
---
After it, looking at something like
if (!func()) {} / if (func()) {}
I'll have to always jump to function definition, to check is it int or bool
function, to understand what exactly is meant and is there a mistake in the
code..
So, I'm afraid that such conversion will not help reviewing/understanding the
code. I'd prefer to avoid using two opposite conventions in on project.
C sucks :)
Conventions help mitigate. Here's one: when fun() returns
non-negative/negative on success/error, always use
fun(...) < 0
This reduces chances that it fit in one line..
But yes, if all use this convention, it makes it obvious what happening.
or
fun(...) >= 0
to check. I dislike the latter.
When returns 0/negative, always use
fun(...) < 0
Avoid
fun(...) >= 0
because that suggests it could return a positive value, which is wrong.
Avoid
fun(...)
because that requires the reader to know the return type.
Exactly the problem I mention. To follow your suggestion, we'll have to update
the whole code base.. However, why not.
When its returns non-null/null or true/false on success/failure, always
use
!fun(...)
Avoid
fun(...)
because that requires the reader to know the return type.
Note that we have a usable check for failure in all cases. Goes well
with the habit to handle errors like this whenever practical
if (failed) {
handle failure
bail out
}
handle success
OK. If convert everything to your suggestion it looks good.
The only possible problem is boolean functions, which just check something, not
returning the error..
With a function like is_x_in_set(x, set), it's native to write
if (is_x_in_set(x, set)) {
...
}
which is a bit in conflict with your suggestion. Still, such functions should
be simply distinguished by name.
I can also imagine combining different function types (int/bool) in if
conditions o_O, what will save us from it?
Can you give an example?
I just meant something like if (f1() || !f2()) { < error > }, nothing special.
And don't forget about bool functions, which just check something, and false is
not an error, but just negative answer on some question.
For what it's worth, GLib specifically advises against such function
contracts:
By convention, if you return a boolean value indicating success then
TRUE means success and FALSE means failure. Avoid creating
functions which have a boolean return value and a GError parameter,
but where the boolean does something other than signal whether the
GError is set. Among other problems, it requires C callers to
allocate a temporary error. Instead, provide a "gboolean *" out
parameter. There are functions in GLib itself such as
g_key_file_has_key() that are deprecated because of this.
Sounds good. But we are speaking about all functions, not only with errp
parameter.. Or not?
--
Best regards,
Vladimir
