| Message ID | 08ee0d6b-788b-2845-6964-e1e55c2d2292@users.sourceforge.net (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
err = snd_card_register(card); > - if (err < 0) { > + if (err) { > +free_card: > snd_card_free(card); > return err; > } I thought we agreed, yesterday, to never use this style of error handling? regards, dan carpenter
On Tue, 22 Aug 2017 14:16:25 +0200, Dan Carpenter wrote: > > err = snd_card_register(card); > > - if (err < 0) { > > + if (err) { > > +free_card: > > snd_card_free(card); > > return err; > > } > > I thought we agreed, yesterday, to never use this style of error > handling? Yeah, this is definitely no-go. Also, please don't omit the negative value check. thanks, Takashi
> Also, please don't omit the negative value check.
Is it appropriate to treat non-zero values as error codes there generally?
Regards,
Markus
On Tue, 22 Aug 2017 14:47:20 +0200, SF Markus Elfring wrote: > > > Also, please don't omit the negative value check. > > Is it appropriate to treat non-zero values as error codes there generally? No, it can't be in general. Lots of functions return a positive value, too. Takashi
>> Is it appropriate to treat non-zero values as error codes there generally? > > No, it can't be in general. I got the impression that the functions which are called at the updated places by the function “snd_card_pcsp_probe” indicate a successful execution only by zero so far. > Lots of functions return a positive value, too. Would you like to point any example out from the programming interface? Regards, Markus
On Tue, 22 Aug 2017 15:15:02 +0200, SF Markus Elfring wrote: > > >> Is it appropriate to treat non-zero values as error codes there generally? > > > > No, it can't be in general. > > I got the impression that the functions which are called at the updated places > by the function “snd_card_pcsp_probe” indicate a successful execution > only by zero so far. You have the impression, great. And what's the reason to drop the negative check? It's not clearer, not better readable. And, the worst part is that you've done it silently even without mentioning in the change log at all. That's really bad. Just don't do it. > > Lots of functions return a positive value, too. > > Would you like to point any example out from the programming interface? For example, the control API functions may return the positive number when the value got changed, 0 for else, and a negative number for the error. The functions returning some numbers may return positive numbers, of course. Takashi
>> I got the impression that the functions which are called at the updated places >> by the function “snd_card_pcsp_probe” indicate a successful execution >> only by zero so far. > > You have the impression, great. This aspect is also a general programming interface issue for some functions. > And what's the reason to drop the negative check? * I find it a bit safer when the error predicate is “return value != 0”. * It is also a small source code reduction. > It's not clearer, not better readable. It seems that we have got different development opinions this time. > And, the worst part is that you've done it silently even without > mentioning in the change log at all. That's really bad. > Just don't do it. I found it not relevant enough for the commit message. > For example, the control API functions may return the positive number > when the value got changed, 0 for else, and a negative number for the > error. The functions returning some numbers may return positive > numbers, of course. Did I touch any specific function calls which belong to this programming interface category? Regards, Markus
On Tue, 22 Aug 2017 16:36:35 +0200, SF Markus Elfring wrote: > > >> I got the impression that the functions which are called at the updated places > >> by the function “snd_card_pcsp_probe” indicate a successful execution > >> only by zero so far. > > > > You have the impression, great. > > This aspect is also a general programming interface issue for some functions. > > > > And what's the reason to drop the negative check? > > * I find it a bit safer when the error predicate is “return value != 0”. Can't agree. And I have no interest to continue bike-shedding, sorry. You can't convince me regarding this. Takashi
>> * I find it a bit safer when the error predicate is “return value != 0”. > > Can't agree. How do you think about to reduce the probability that positive return values will accidentally be interpreted as a successful function execution. > And I have no interest to continue bike-shedding, sorry. I do not like that you prefer to put this technical detail into such a communication category. > You can't convince me regarding this. Would you still like to integrate the proposed refactoring with the use of previous failure predicates then? Regards, Markus
On Tue, 22 Aug 2017 17:03:00 +0200, SF Markus Elfring wrote: > > >> * I find it a bit safer when the error predicate is “return value != 0”. > > > > Can't agree. > > How do you think about to reduce the probability that positive return values > will accidentally be interpreted as a successful function execution. It's not zero. > > And I have no interest to continue bike-shedding, sorry. > > I do not like that you prefer to put this technical detail into such > a communication category. > > > > You can't convince me regarding this. > > Would you still like to integrate the proposed refactoring with the use > of previous failure predicates then? That's fine. But, please don't forget what others already mentioned. For example, Joe Perches suggested to put a blank line before the label for your patches. But you completely ignored it and did the same again. thanks, Takashi
diff --git a/sound/drivers/pcsp/pcsp.c b/sound/drivers/pcsp/pcsp.c index 72e2d0012084..c8ea4fed3905 100644 --- a/sound/drivers/pcsp/pcsp.c +++ b/sound/drivers/pcsp/pcsp.c @@ -108,22 +108,17 @@ static int snd_card_pcsp_probe(int devnum, struct device *dev) return err; err = snd_pcsp_create(card); - if (err < 0) { - snd_card_free(card); - return err; - } + if (err) + goto free_card; + if (!nopcm) { err = snd_pcsp_new_pcm(&pcsp_chip); - if (err < 0) { - snd_card_free(card); - return err; - } + if (err) + goto free_card; } err = snd_pcsp_new_mixer(&pcsp_chip, nopcm); - if (err < 0) { - snd_card_free(card); - return err; - } + if (err) + goto free_card; strcpy(card->driver, "PC-Speaker"); strcpy(card->shortname, "pcsp"); @@ -131,7 +126,8 @@ static int snd_card_pcsp_probe(int devnum, struct device *dev) pcsp_chip.port); err = snd_card_register(card); - if (err < 0) { + if (err) { +free_card: snd_card_free(card); return err; }