Vladimir Sementsov-Ogievskiy <vsement...@virtuozzo.com> writes: > 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> > }
Yes, when you actually need the pointer. Common, but not universal. > 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.. Yes, that's a drawback. > 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. Only if we value the consistency more than the update labor and churn. >> >> 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. The conventions I described only apply to error checking. I certainly don't mean to discourage things like while (*p && qemu_isspace(*p)) { p++; } Also, they're *conventions*, not law. The purpose of conventions is to help us write clearer code. Don't make code less clear just to conform to a convention. Use your judgement. >>> 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. Perfectly consistent error checking style, idiomatic C, pick one. >>> 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? The conventions I described apply to error checking, regardless of Error object use. For instance, they'd apply to error-checking remove("some-file").