Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > comp.lang.c > #55792 > unrolled thread

Is this poor style?

Back to article view | Back to comp.lang.c

Contents

  Is this poor style? "S.K.R. de Jong" <SKRdJ@nowhere.net> - 2015-01-04 00:46 +0000
    Re: Is this poor style? Malcolm McLean <malcolm.mclean5@btinternet.com> - 2015-01-03 16:58 -0800
      Re: Is this poor style? Ian Collins <ian-news@hotmail.com> - 2015-01-04 20:37 +1300
        Re: Is this poor style? Malcolm McLean <malcolm.mclean5@btinternet.com> - 2015-01-04 02:32 -0800
          Re: Is this poor style? Ian Collins <ian-news@hotmail.com> - 2015-01-05 09:39 +1300
            Re: Is this poor style? Jorgen Grahn <grahn+nntp@snipabacken.se> - 2015-01-04 22:53 +0000
    Re: Is this poor style? Ben Bacarisse <ben.usenet@bsb.me.uk> - 2015-01-04 01:29 +0000
    Re: Is this poor style? Morris Dovey <mrdovey@iedu.com> - 2015-01-04 00:00 -0600
      Re: Is this poor style? Morris Dovey <mrdovey@iedu.com> - 2015-01-04 02:20 -0600
    Re: Is this poor style? August Karlstrom <fusionfile@gmail.com> - 2015-01-04 15:46 +0100
      Re: Is this poor style? Ike Naar <ike@iceland.freeshell.org> - 2015-01-04 15:10 +0000
        Re: Is this poor style? August Karlstrom <fusionfile@gmail.com> - 2015-01-04 17:06 +0100
    Re: Is this poor style? Clark Smith <noaddress@nowhere.net> - 2015-01-04 15:30 +0000
      Re: Is this poor style? Malcolm McLean <malcolm.mclean5@btinternet.com> - 2015-01-04 07:44 -0800
        Re: Is this poor style? Richard Heathfield <invalid@see.sig.invalid> - 2015-01-05 00:35 +0000
          Re: Is this poor style? BartC <bc@freeuk.com> - 2015-01-05 01:02 +0000
            Re: Is this poor style? Richard Heathfield <invalid@see.sig.invalid> - 2015-01-05 01:43 +0000
              Re: Is this poor style? BartC <bc@freeuk.com> - 2015-01-05 11:35 +0000
              Re: Is this poor style? raltbos@xs4all.nl (Richard Bos) - 2015-01-05 12:58 +0000
                Re: Is this poor style? Richard Heathfield <invalid@see.sig.invalid> - 2015-01-05 13:14 +0000
            Re: Is this poor style? Jorgen Grahn <grahn+nntp@snipabacken.se> - 2015-01-05 22:02 +0000
              Re: Is this poor style? Malcolm McLean <malcolm.mclean5@btinternet.com> - 2015-01-05 14:24 -0800
                Re: Is this poor style? Ian Collins <ian-news@hotmail.com> - 2015-01-06 12:26 +1300
                  Re: Is this poor style? Malcolm McLean <malcolm.mclean5@btinternet.com> - 2015-01-06 02:40 -0800
                Re: Is this poor style? Jorgen Grahn <grahn+nntp@snipabacken.se> - 2015-01-07 19:32 +0000
                  Re: Is this poor style? Richard Heathfield <invalid@see.sig.invalid> - 2015-01-07 20:03 +0000
                    Re: Is this poor style? BartC <bc@freeuk.com> - 2015-01-07 20:27 +0000
                      Re: Is this poor style? Ian Collins <ian-news@hotmail.com> - 2015-01-08 09:55 +1300
                        Re: Is this poor style? Les Cargill <lcargill99@comcast.com> - 2015-01-07 18:13 -0600
                          Re: Is this poor style? Ian Collins <ian-news@hotmail.com> - 2015-01-08 13:17 +1300
                      Re: Is this poor style? Richard Heathfield <invalid@see.sig.invalid> - 2015-01-07 21:02 +0000
                        Re: Is this poor style? Rosario193 <Rosario@invalid.invalid> - 2015-01-08 11:22 +0100
                          Re: Is this poor style? Richard Heathfield <invalid@see.sig.invalid> - 2015-01-08 11:33 +0000
                            Re: Is this poor style? Rosario193 <Rosario@invalid.invalid> - 2015-01-08 13:08 +0100
                              Re: Is this poor style? Richard Heathfield <invalid@see.sig.invalid> - 2015-01-08 12:54 +0000
                        Re: Is this poor style? David Brown <david.brown@hesbynett.no> - 2015-01-08 14:04 +0100
                      Re: Is this poor style? Keith Thompson <kst-u@mib.org> - 2015-01-07 13:57 -0800
                      Re: Is this poor style? Les Cargill <lcargill99@comcast.com> - 2015-01-07 18:12 -0600
                    Re: Is this poor style? Ben Bacarisse <ben.usenet@bsb.me.uk> - 2015-01-07 20:56 +0000
                      Re: Is this poor style? Richard Heathfield <invalid@see.sig.invalid> - 2015-01-07 21:18 +0000
                    Re: Is this poor style? glen herrmannsfeldt <gah@ugcs.caltech.edu> - 2015-01-07 21:28 +0000
              Re: Is this poor style? BartC <bc@freeuk.com> - 2015-01-06 11:28 +0000
                Re: Is this poor style? Jorgen Grahn <grahn+nntp@snipabacken.se> - 2015-01-07 20:19 +0000
                  Re: Is this poor style? Malcolm McLean <malcolm.mclean5@btinternet.com> - 2015-01-07 16:05 -0800
                    Re: Is this poor style? glen herrmannsfeldt <gah@ugcs.caltech.edu> - 2015-01-08 08:29 +0000
                    Re: Is this poor style? Jorgen Grahn <grahn+nntp@snipabacken.se> - 2015-01-12 22:08 +0000
                      Re: Is this poor style? Tim Rentsch <txr@alumni.caltech.edu> - 2015-03-24 20:35 -0700
                        Re: Is this poor style? Malcolm McLean <malcolm.mclean5@btinternet.com> - 2015-03-25 08:43 -0700
                          Re: Is this poor style? Tim Rentsch <txr@alumni.caltech.edu> - 2015-03-25 16:38 -0700
                          Re: Is this poor style? Stephen Sprunk <stephen@sprunk.org> - 2015-03-26 15:43 -0500
                            Re: Is this poor style? Tim Rentsch <txr@alumni.caltech.edu> - 2015-03-28 08:57 -0700
                  Re: Is this poor style? Ian Collins <ian-news@hotmail.com> - 2015-01-08 13:23 +1300
                    Re: Is this poor style? Malcolm McLean <malcolm.mclean5@btinternet.com> - 2015-01-07 16:53 -0800
                      Re: Is this poor style? Ian Collins <ian-news@hotmail.com> - 2015-01-08 14:11 +1300
                Re: Is this poor style? Ian Collins <ian-news@hotmail.com> - 2015-01-08 11:10 +1300
          Re: Is this poor style? John Bode <jfbode1029@gmail.com> - 2015-01-05 15:54 -0800
            Re: Is this poor style? Rosario193 <Rosario@invalid.invalid> - 2015-01-06 11:03 +0100
              Re: Is this poor style? Rosario193 <Rosario@invalid.invalid> - 2015-01-06 11:10 +0100
              Re: Is this poor style? John Bode <jfbode1029@gmail.com> - 2015-01-06 06:03 -0800
                Re: Is this poor style? Richard Heathfield <invalid@see.sig.invalid> - 2015-01-06 14:44 +0000
                  Re: Is this poor style? "Osmium" <r124c4u102@comcast.net> - 2015-01-06 09:10 -0600
                    Re: Is this poor style? Rosario193 <Rosario@invalid.invalid> - 2015-01-08 11:14 +0100
                      Re: Is this poor style? Richard Heathfield <invalid@see.sig.invalid> - 2015-01-08 11:31 +0000
                        Re: Is this poor style? Rosario193 <Rosario@invalid.invalid> - 2015-01-08 13:06 +0100
                          Re: Is this poor style? Rosario193 <Rosario@invalid.invalid> - 2015-01-08 13:19 +0100
                          Re: Is this poor style? Richard Heathfield <invalid@see.sig.invalid> - 2015-01-08 12:53 +0000
                            Re: Is this poor style? Malcolm McLean <malcolm.mclean5@btinternet.com> - 2015-01-08 05:49 -0800
                      Re: Is this poor style? Keith Thompson <kst-u@mib.org> - 2015-01-08 08:18 -0800
                        Re: Is this poor style? Rosario193 <Rosario@invalid.invalid> - 2015-01-09 07:58 +0100
                        Re: Is this poor style? Tim Rentsch <txr@alumni.caltech.edu> - 2015-03-24 09:28 -0700
                          Re: Is this poor style? Keith Thompson <kst-u@mib.org> - 2015-03-24 13:57 -0700
                            Re: Is this poor style? Tim Rentsch <txr@alumni.caltech.edu> - 2015-03-25 08:28 -0700
                              Re: Is this poor style? Keith Thompson <kst-u@mib.org> - 2015-03-25 12:17 -0700
                            Re: Is this poor style? Öö Tiib <ootiib@hot.ee> - 2015-04-05 17:03 -0700
                      Re: Is this poor style? John Bode <jfbode1029@gmail.com> - 2015-01-14 11:25 -0800
                        Re: Is this poor style? Richard Heathfield <invalid@see.sig.invalid> - 2015-01-14 19:41 +0000
                        Re: Is this poor style? Rosario193 <Rosario@invalid.invalid> - 2015-01-15 09:01 +0100
                        Re: Is this poor style? Rosario193 <Rosario@invalid.invalid> - 2015-01-15 09:35 +0100
      Re: Is this poor style? James Kuyper <jameskuyper@verizon.net> - 2015-01-04 11:15 -0500
        Re: Is this poor style? Jorgen Grahn <grahn+nntp@snipabacken.se> - 2015-01-04 21:38 +0000
        Re: Is this poor style? Dr Nick <nospam-4@temporary-address.org.uk> - 2015-01-05 07:27 +0000
          Re: Is this poor style? Malcolm McLean <malcolm.mclean5@btinternet.com> - 2015-01-05 06:33 -0800
      Re: Is this poor style? Vincenzo Mercuri <nomail@yahoo.it> - 2015-01-04 18:56 +0100
      Re: Is this poor style? rouben@shady.(none) (Rouben Rostamian) - 2015-01-04 18:22 +0000
      Re: Is this poor style? Ben Bacarisse <ben.usenet@bsb.me.uk> - 2015-01-04 21:35 +0000
      Re: Is this poor style? Ian Collins <ian-news@hotmail.com> - 2015-01-05 10:46 +1300
        Re: Is this poor style? Malcolm McLean <malcolm.mclean5@btinternet.com> - 2015-01-04 15:36 -0800
      Re: Is this poor style? asetofsymbols@gmail.com - 2015-01-05 12:05 -0800
        Re: Is this poor style? asetofsymbols@gmail.com - 2015-01-05 14:18 -0800
          Re: Is this poor style? asetofsymbols@gmail.com - 2015-01-05 14:32 -0800
            Re: Is this poor style? asetofsymbols@gmail.com - 2015-01-05 14:37 -0800
          Re: Is this poor style? James Kuyper <jameskuyper@verizon.net> - 2015-01-05 18:23 -0500
            Re: Is this poor style? Richard Heathfield <invalid@see.sig.invalid> - 2015-01-05 23:32 +0000
        Re: Is this poor style? Keith Thompson <kst-u@mib.org> - 2015-01-05 15:31 -0800
      Re: Is this poor style? Phil Carmody <pc+usenet@asdf.org> - 2015-01-13 15:00 +0200
    Re: Is this poor style? Les Cargill <lcargill99@comcast.com> - 2015-01-04 15:07 -0600
    Re: Is this poor style? jadill33@gmail.com - 2015-01-05 15:03 -0800
    Re: Is this poor style? buja <buja66@gmail.com> - 2015-01-06 21:48 +0100
      Re: Is this poor style? Rosario193 <Rosario@invalid.invalid> - 2015-01-08 11:21 +0100
        Re: Is this poor style? buja <buja66@gmail.com> - 2015-01-08 12:42 +0100
    Re: Is this poor style? "S.K.R. de Jong" <SKRdJ@nowhere.net> - 2015-01-08 15:29 +0000
      Re: Is this poor style? James Kuyper <jameskuyper@verizon.net> - 2015-01-08 12:03 -0500

Page 5 of 6 — ← Prev page 1 2 3 4 [5] 6  Next page →

#55874

James Kuyper <jameskuyper@verizon.net> writes:

> On 01/04/2015 10:30 AM, Clark Smith wrote:
>> On Sun, 04 Jan 2015 00:46:44 +0000, S.K.R. de Jong wrote:
>> 
>>> I would like the opinion of people in this forum as to whether or not
>>> the following is poor style:
>>>
>>> 	int f(char *x)
>>> 	{
>>> 		int diagnostic = 0;
>>>
>>> 		do {
>>> 			if (x == NULL)
>>> 				break;
>>>
>>> 			...
>>>
>>> 			diagnostic = 1;
>>>
>>> 		} while(0);
>>>
>>> 		return diagnostic;
>>> 	}
>>>
>>> The same could be accomplished with goto, of course. Which kind of
>>> construct is more acceptable for dealing with error conditions: the
>>> empty while loop? goto with labels? Something else?
>> 
>> 	I thought that the ellipsis would convey what I meant, but that 
>> does not seem to have been the case. Here is a nonsensical, toy example 
>> that illustrates better what I have in mind:
>> 
>> 	int f(char *x, int l)
>> 	{
>> 		char *a = NULL, *b = NULL;
>> 		int diagnostic = 0;
>> 
>> 		do {
>> 			if (x == NULL)
>> 				break;
>> 
>> 			if (l > 255)
>> 				break;
>> 
>> 			a = malloc(256);
>> 			if (a == NULL)
>> 				break;
>> 
>> 			b = malloc(2*256);
>> 			if (b == NULL)
>> 				break;
>> 
>> 			strcpy(a, x);
>> 			strcpy(b, x);
>> 			strcpy(&b[255], a);
>> 
>> 			diagnostic = 1;
>> 		} while(0);
>> 
>> 		if (diagnostic == 0) {
>> 			if (a != NULL)
>> 				free(a);
>> 			if (b != NULL)
>> 				free(b);
>> 		}
>> 
>> 		return diagnostic;
>> 	}
>> 
>> 
>> The idea is that some actions must be taken that must be correctly undone 
>> in case of error.
>
> That is an essential point; the fact that it's missing from your
> original example is why the answers you received were not to the point.
>
> Here's how I would have handled your toy example:
>
> 	int f(char *x, int l)
> 	{
>                 if(x==NULL || l > 255)
>                     return 0;
>
> 		int diagnostic = 0;
>                 char *a = malloc(256);
>
>                 if(a==NULL)
>                     return 0;
>
>                 char *b = malloc(2*256);
>
>                 if(b)
>                 {
> 			strcpy(a, x);
> 			strcpy(b, x);
> 			strcpy(&b[255], a);
>                         free(b);
>                         diagnostic = 1;
>                 }
>                 free(a);
>
> 		return diagnostic;
> 	}
>
> This structure makes it easy to verify that a is free()d only if
> successfully allocated, b is free()d only if successfully allocated, and
> the core part of the code is executed only if both are successfully
> allocated.

Me too.  It seems there are four options:
a) bail out of a function early under clearly defined "error" conditions
b) use lots of nested ifs (and probably status variables)
c) use a distorted loop structure (and probably status variables)
d) use goto

The objections to (a) that are often presented seem to me a matter of
dogma rather than pragmatism - "a function should only have one return".
Given the alternatives available, this is in many cases - IMO of course
- the least evil.

[toc] | [prev] | [next] | [standalone]

#55897

On Monday, January 5, 2015 7:26:20 AM UTC, Dr Nick wrote:
> 
> Me too.  It seems there are four options:
> a) bail out of a function early under clearly defined "error" conditions
> b) use lots of nested ifs (and probably status variables)
> c) use a distorted loop structure (and probably status variables)
> d) use goto
> 
> The objections to (a) that are often presented seem to me a matter of
> dogma rather than pragmatism - "a function should only have one return".
> Given the alternatives available, this is in many cases - IMO of course
> 
Option e)

Separate out all the IO routines from the bit-shuffling functions (pure functions, functions,
Malcolm-functions has even been suggested, whatever you want to call things that don't
do IO).
Now write a wrapper to malloc that means that malloc never returns NULL - obviously
it's physically impossible to make sure that every request for memory will be honoured,
but you put up a dialog saying "please shut down your other applications to give me
more memory or kill me now" sort of thing, a teenage girl emotional blackmail sort of
interface.

Now you can't have any error in the bit-shullfing functions that are not programming errors. 
(Functions which are parsers are a bit of a special case, it's almost as hard to check the input 
for valid grammar as it is to write the parser itself, which makes the parser useless, so error
in input to a parser has to be treated as normal control flow).
If you've got a programming error then there is by definition no correct behaviour
possible for the program, so you don't need to worry about it.

Now you don't need gotos.

[toc] | [prev] | [next] | [standalone]

#55827

Il 04/01/2015 16:30, Clark Smith ha scritto:
[..]
> 			strcpy(a, x);
> 			strcpy(b, x);
> 			strcpy(&b[255], a);

Did you mean 256?

[..]
> 		if (diagnostic == 0) {
> 			if (a != NULL)
> 				free(a);
> 			if (b != NULL)
> 				free(b);

Don't worry about testing whether 'a' or 'b' are NULL; in those
cases free() just takes no action.

Something like:

#include <stdlib.h>
#include <string.h>

int f(char* x, int l)
{
     char *a = NULL;
     char *b = NULL;
     int diagnostic = 0;

     if(x == NULL || l > 255) return diagnostic;

     a = malloc(256);
     if(a) strcpy(a, x); else goto cleanup;

     b = malloc(2 * 256);
     if(b) strcpy(b, x); else goto cleanup;

     strcpy(&b[256], a);
     diagnostic = 1;

cleanup:
     free(a);
     free(b);
     return diagnostic;
}

int main(void)
{
     return !f("Hello World!", 0);
}

would make your intent clearer to me.

-- 
Vincenzo Mercuri

[toc] | [prev] | [next] | [standalone]

#55830

In article <m8bmb9$cg3$1@news.albasani.net>,
Clark Smith  <noaddress@nowhere.net> wrote:
>	I thought that the ellipsis would convey what I meant, but that 
>does not seem to have been the case. Here is a nonsensical, toy example 
>that illustrates better what I have in mind:
>
>	int f(char *x, int l)
>	{
>		char *a = NULL, *b = NULL;
>		int diagnostic = 0;
>
>		do {
>			if (x == NULL)
>				break;
>
>			if (l > 255)
>				break;
>
>			a = malloc(256);
>			if (a == NULL)
>				break;
>
>			b = malloc(2*256);
>			if (b == NULL)
>				break;
>
>			strcpy(a, x);
>			strcpy(b, x);
>			strcpy(&b[255], a);
>
>			diagnostic = 1;
>		} while(0);
>
>		if (diagnostic == 0) {
>			if (a != NULL)
>				free(a);
>			if (b != NULL)
>				free(b);
>		}
>
>		return diagnostic;
>	}
>
>
>The idea is that some actions must be taken that must be correctly undone 
>in case of error. With this approach, everything is centralized at the 
>end of the loop. 

I believe that the code should express the writer's true intent
rather than show a self-conscious attempt in evading the use
of goto based on a certain vague dogma.

I think the following is a more expressive version of your code:

int f(char *x, int l)
{
        char *a=NULL, *b=NULL;
        diagnostic = 0;
        if (x == NULL || l > 255)
                goto cleanup;
        if ((a = malloc(256))== NULL || (b = malloc(2*256))== NULL)
                goto cleanup;
        strcpy(a, x);
        strcpy(b, x);
        strcpy(b+256, a);
        diagnostic = 1;
cleanup:
        free(a);
        free(b);
        return diagnostic;
}

Remark 1:
I have fixed a potential memory leak present in your code which
fails to free the memory allocated to a and b if diagnostic is 1.

Remark 2:
There is no point in testing `if (a != NULL)' and `if (b != NULL)'
since free(NULL) is a perfectly fine no-op in C.

-- 
Rouben Rostamian

[toc] | [prev] | [next] | [standalone]

#55841

Clark Smith <noaddress@nowhere.net> writes:
<snip>
> 	I thought that the ellipsis would convey what I meant, but that 
> does not seem to have been the case.

It seemed to say "normal code goes here".

> Here is a nonsensical, toy example 
> that illustrates better what I have in mind:

The trouble is that toy example have toy solutions, and real examples
often have entirely different real solutions.  As you make more and more
complex conditions to illustrate what you mean, you will eventually tip
over into a situation where writing separate functions starts to make
sense.

Having said that I a pretty sure that I would never want to use break
inside a do .. while (0); loop.

> 	int f(char *x, int l)
> 	{
> 		char *a = NULL, *b = NULL;
> 		int diagnostic = 0;
>
> 		do {
> 			if (x == NULL)
> 				break;
>
> 			if (l > 255)
> 				break;
>
> 			a = malloc(256);
> 			if (a == NULL)
> 				break;
>
> 			b = malloc(2*256);
> 			if (b == NULL)
> 				break;
>
> 			strcpy(a, x);
> 			strcpy(b, x);
> 			strcpy(&b[255], a);
>
> 			diagnostic = 1;
> 		} while(0);
>
> 		if (diagnostic == 0) {
> 			if (a != NULL)
> 				free(a);
> 			if (b != NULL)
> 				free(b);
> 		}
>
> 		return diagnostic;
> 	}

  char *a = NULL, *b = NULL;

  if (x != NULL && l <= 255 &&
      (a = malloc(256)) && (b = malloc(2*256))) {
         strcpy(a, x);
         strcpy(b, x);
         strcpy(&b[255], a);
         return 1;
  }
  free(a);
  free(b);
  return 0;

Barring mistakes, this duplicates the behaviour (including the memory
leak!).

> The idea is that some actions must be taken that must be correctly undone 
> in case of error. With this approach, everything is centralized at the 
> end of the loop. 

I prefer to determine the "all good to go condition" explicitly.

-- 
Ben.

[toc] | [prev] | [next] | [standalone]

#55845

Clark Smith wrote:
>
> 	I thought that the ellipsis would convey what I meant, but that
> does not seem to have been the case. Here is a nonsensical, toy example
> that illustrates better what I have in mind:
>
> 	int f(char *x, int l)
> 	{
> 		char *a = NULL, *b = NULL;
> 		int diagnostic = 0;
>
> 		do {
> 			if (x == NULL)
> 				break;
>
> 			if (l > 255)
> 				break;

These are preconditions, they should be tested at the top of the 
function before you do anything else.  Handling their failure along with 
other failures adds unnecessary complexity and hides what they are.

-- 
Ian Collins

[toc] | [prev] | [next] | [standalone]

#55854

On Sunday, January 4, 2015 9:47:00 PM UTC, Ian Collins wrote:
> 
> These are preconditions, they should be tested at the top of the 
> function before you do anything else.  Handling their failure along with 
> other failures adds unnecessary complexity and hides what they are.
> 
I did think of making that point. Why not assert test the pointer?

However if the spec says return 0, that's what you must do.

[toc] | [prev] | [next] | [standalone]

#55948

Yes poor style... f() function is useless ...
 So allocate 2 string and return 1 if allocation os ok?
 (without the use of allocation) than I would not use variable 'l' because it seems 1 to me... 
I would not use 'p' and 'b' together
too

[toc] | [prev] | [next] | [standalone]

#55959

I would write it so:

#include <stdlib.h> 
#include <string.h> 

int f(char* x, int    c) 
{ char *a = 0, *b =0; 
  int            r=0;
  if(x==0 || c>255) return 0; 
  if(c!=strlen(x))  return 0;
  if((a=malloc(512))==0)   
                    return 0;
  if((b=malloc(2*512))==0) 
          {free(a); return 0;}
  strcpy(a, x); 
  strcpy(b, x); 
  strcpy(b+258, a);
...... Operations modify r
  free(a);
  free(b);
  return  r;
} 

[toc] | [prev] | [next] | [standalone]

#55961

Perhaps better:

#include <stdlib.h> 
#include <string.h> 
#define G goto

int f(char* x, int    c) 
{ char *a = 0, *b =0; 
  int            r=0; 
  if(x==0 || c>255) 
    {ex:  return 0; }
  if(c!=strlen(x))        G ex; 
  if((a=malloc(512))==0)  G ex; 
  if((b=malloc(2*512))==0) 
           {ex1: free(a); G ex;} 
  strcpy(a, x); 
  strcpy(b, x); 
  strcpy(b+258, a); 
...... Operations modify r 
       if some error found G ex1
  free(a); 
  free(b); 
  return  r; 
}
There are 2 point of exit routine
1) all ok not find error
2) error found

[toc] | [prev] | [next] | [standalone]

#55962

Perhaps better:

#include <stdlib.h>
#include <string.h>
#define G goto

int f(char* x, int c)
{ char *a = 0, *b =0;
int r=0;
if(x==0 || c>255)
{ex: return 0; }
if(c!=strlen(x)) G ex;
if((a=malloc(512))==0) G ex;
if((b=malloc(2*512))==0)
{ex1: free(a); G ex;}
strcpy(a, x);
strcpy(b, x);
strcpy(b+258, a);
...... Operations modify r
if some error found G ex1
free(b);
free(a);
return r;
}
There are 2 point of exit routine
1) all ok not find error
2) error found

[toc] | [prev] | [next] | [standalone]

#55965

On 01/05/2015 05:18 PM, asetofsymbols@gmail.com wrote:
...
>   if((a=malloc(512))==0)   
>                     return 0;
>   if((b=malloc(2*512))==0) 
>           {free(a); return 0;}
...
>   free(a);

I prefer, when possible, to make sure that there's one and only one call
to free() in the code for a given block of memory. This makes it easier
to check that there is one and only execution of the free() call, for
each successful call to malloc(). This isn't always feasible - which is
a perfectly reasonable reason for using some other approach - but it's
trivially feasible in this case.
-- 
James Kuyper

[toc] | [prev] | [next] | [standalone]

#55968

James Kuyper wrote:

> On 01/05/2015 05:18 PM, asetofsymbols@gmail.com wrote:
> ...
>>   if((a=malloc(512))==0)
>>                     return 0;
>>   if((b=malloc(2*512))==0)
>>           {free(a); return 0;}
> ...
>>   free(a);
> 
> I prefer, when possible, to make sure that there's one and only one call
> to free() in the code for a given block of memory. This makes it easier
> to check that there is one and only execution of the free() call, for
> each successful call to malloc(). This isn't always feasible - which is
> a perfectly reasonable reason for using some other approach - but it's
> trivially feasible in this case.

In fact, I prefer, where possible, to bundle the allocation into a separate 
function:

T *t_create(args)
{
  T *new = malloc(sizeof *new);
  if(new != NULL)
  {
    /* initialise... */
  }
  return new;
}

and destruction into a different function:

void t_destroy(T **old)
{
  if(old)
  {
    /* deal with any child objects, and then... */
    free(*old);
    *old = NULL;
  }
}

T *p = t_create(args);
if(p != NULL)
{
  /* code... */
  /* later... */
  t_destroy(&p);

This meets your objective, and if it turns out to be convenient to include 
t_destroy() in more than one place, no harm is done - the memory will only 
be freed once.

-- 
Richard Heathfield
Email: rjh at cpax dot org dot uk
"Usenet is a strange place" - dmr 29 July 1999
Sig line 4 vacant - apply within

[toc] | [prev] | [next] | [standalone]

#55967

asetofsymbols@gmail.com writes:
> Yes poor style... f() function is useless ...
>  So allocate 2 string and return 1 if allocation os ok?
>  (without the use of allocation) than I would not use variable 'l' because it seems 1 to me... 
> I would not use 'p' and 'b' together
> too

When you post a followup, please quote enough of the message you're
replying to so that your followup makes sense.  See most of the articles
in this newsgroup for examples.  (And please don't post off-topic
articles like your earlier one about the UFO sighting.)

Thanks.

-- 
Keith Thompson (The_Other_Keith) kst-u@mib.org  <http://www.ghoti.net/~kst>
Working, but not speaking, for JetHead Development, Inc.
"We must do something.  This is something.  Therefore, we must do this."
    -- Antony Jay and Jonathan Lynn, "Yes Minister"

[toc] | [prev] | [next] | [standalone]

#56201

ram@zedat.fu-berlin.de (Stefan Ram) writes:
> ram@zedat.fu-berlin.de (Stefan Ram) writes:
> >static inline int copy
> >( char const * const x, char * const a, char * const b )
> >{ int const success = 1; strcpy( a, x ); strcpy( b, x );
> >  strcpy( b + 255, a ); return success; }
> 
>   Why did I not write »return 1;«? Readability!
> 
>   Look at this line:
> 
> qsort( s, strlen( s ), 1, comp ); 
> 
>   . Do you remember what the »1« means?
> 
>   The expression »sizeof( char )« is despised by some,
>   but it makes the meaning clearer.
>   
> qsort( s, strlen( s ), sizeof( char ), comp ); 

That presumes s is a char*; sizeof(*s) doesn't. ere you to change
s to point to a wider character type, you'd need to change the
sizeof line. With the above alternative, you wouldn't.

Phil
-- 
A well regulated militia, being necessary to the security of a free state,
the right of the people to keep and bear arms, shall be well regulated.

[toc] | [prev] | [next] | [standalone]

#55838

S.K.R. de Jong wrote:
> 	I would like the opinion of people in this forum as to whether or
> not the following is poor style:
>
> 	int
> 	f(char *x)
> 	{
> 		int diagnostic = 0;
>
> 		do {
> 			if (x == NULL)
> 				break;
>
> 			...
>
> 			diagnostic = 1;
>
> 		} while(0);
>
> 		return diagnostic;
> 	}
>
> The same could be accomplished with goto, of course. Which kind of
> construct is more acceptable for dealing with error conditions: the empty
> while loop? goto with labels? Something else?
>
>


Not enough information to know. You example is too incomplete to judge.

I like "while (1) {...} " loops with "break" for error conditions.
The allows you to enumerate checked-for error cases in a nicely linear
fashion, with the "happy path" at the bottom.


"do {
...
} while (0);"

Is sorta weird.

YMMV.

-- 
Les Cargill

[toc] | [prev] | [next] | [standalone]

#55963

On Saturday, January 3, 2015 7:46:54 PM UTC-5, S.K.R. de Jong wrote:
> I would like the opinion of people in this forum as to whether or 
> not the following is poor style:
> 
> 	int
> 	f(char *x)
> 	{
> 		int diagnostic = 0;
> 
> 		do {
> 			if (x == NULL)
> 				break;
> 
> 			...
> 
> 			diagnostic = 1;
> 
> 		} while(0);
> 
> 		return diagnostic;
> 	}
> 
> The same could be accomplished with goto, of course. Which kind of 
> construct is more acceptable for dealing with error conditions: the empty 
> while loop? goto with labels? Something else?

The difficulty with software verification is that there are many
sources of software defects, and many different methods to handle
them.  For example, consider how one should handle an invalid
argument to a function.  Depending on the situation, one could
return an error code, return a neutral value, use the closest
legal value, call an error processing routine, log a warning
message to a file, abort the program, and so on.  The biggest
concern is to ensure that related functions handle errors in a
consistent fashion rather than an ad-hoc collection of different
techniques.

In the development stage, you typically want these situations where
'x == NULL' to fail as soon as they are detected.  This style is
commonly referred to as using contract programming.  In this style,
assert is a common method to verifying the user of a function of
this kind of mistake, which happens to inform the developer of the
exact line and location where this occurred to speed up
troubleshooting.

However, when the software moves closer to release, often times the
most important aspect shifts is to avoid error handling styles that
result in a catastrophic termination of the program.  This style is
commonly referred to as defensive programming.  If 'x == NULL', the
usual response is to early exit using some form of goto: return, break,
or goto itself.

The now beloved assert is looked upon with scorn because now any lapse
in diligence in preventing bugs now results in crashing the program,
which usually has a very negative connotation with the user.

The way I approach the subject is to view the validation of a software
constraint as a generalization of three separate steps: detection,
report, and response.

The detection step evaluates a (boolean) expression that determines
whether the conditions of the constraint are valid.  The reporting
step informs the user or developer that a constraint violation was
detected.  The response step is the action to take after
reporting a constraint violation.  A common response that I usually
see is a defensive programming style is to exit the module as
quickly as possible and returning or setting some error state.  There
are several styles to choose from, but my personal preference is the
early return for parameter error checking 'x == NULL'.

However, in general it is difficult to come up with one style that
satisfies all development phases with its shifting focus.  The
solution I prefer in my own personal code is a macro similar to
GLib's g_return_val_if_fail and g_return_if_fail.  These macros
package the detection, report, and response into a single interface
so that if one wants to change the interface, it is done in one spot,
plus it gives you access to __LINE__, __FILE__ and other preprocessing
goodies.

If I was in your scenario, I would use an early return macro statement
as I feel in general it reduces the amount of clutter to get as much
error handling out of the way so the real work is easier to grok.  A
macro is able to combine several statements into one so the amount of
code space can be significantly reduced.  If I used one of my macros,
the solution would be:

c_return_value_if_fail(x == NULL, 1);

or even better use a #define a la '#define INVALID_PARAMETER 1'.

c_return_value_if_fail(x == NULL, INVALID_PARAMETER);

You can have function pointers for the report and response steps if
you want to provide external control how those errors are captured
under the hood in those macros.

For example, in development, the report and response can be identical
to assert.  In run-time environments, one possibility is that errors
can be logged to a file and the response is saving current progress
and putting up a dialog.  It's something that I've simulated annealed
into over several years of exploring different methodologies.

Best regards,
John D.

[toc] | [prev] | [next] | [standalone]

#55997

On 01/04/2015 01:46 AM, S.K.R. de Jong wrote:
> 	I would like the opinion of people in this forum as to whether or
> not the following is poor style:
>
> 	int
> 	f(char *x)
> 	{
> 		int diagnostic = 0;
>
> 		do {
> 			if (x == NULL)
> 				break;
>
> 			...
>
> 			diagnostic = 1;
>
> 		} while(0);
>
> 		return diagnostic;
> 	}
>
> The same could be accomplished with goto, of course. Which kind of
> construct is more acceptable for dealing with error conditions: the empty
> while loop? goto with labels? Something else?
>
>

Why not return immediately if x == NULL?
Why the loop? Is doesn't seem to be doing anything useful (unless there 
is some iterative calculation on x).

This doesn't need a loop or any goto's, or even the diagnostic variable 
(although it may improve readability):

int f(char *x)
{
     /* check x, if it is a null pointer, return immediately */
     if (x == NULL)
	return 0; /* indicates error */

     /* normal processing */
     ...

     return 1; /* indicates normal processing */
}

[toc] | [prev] | [next] | [standalone]

#56042

On Tue, 06 Jan 2015 21:48:02 +0100, buja <buja66@gmail.com> wrote:

>On 01/04/2015 01:46 AM, S.K.R. de Jong wrote:
>> 	I would like the opinion of people in this forum as to whether or
>> not the following is poor style:
>>
>> 	int
>> 	f(char *x)
>> 	{
>> 		int diagnostic = 0;
>>
>> 		do {
>> 			if (x == NULL)
>> 				break;
>>
>> 			...
>>
>> 			diagnostic = 1;
>>
>> 		} while(0);
>>
>> 		return diagnostic;
>> 	}
>>
>> The same could be accomplished with goto, of course. Which kind of
>> construct is more acceptable for dealing with error conditions: the empty
>> while loop? goto with labels? Something else?
>>
>>
>
>Why not return immediately if x == NULL?
>Why the loop? Is doesn't seem to be doing anything useful (unless there 
>is some iterative calculation on x).
>
>This doesn't need a loop or any goto's, or even the diagnostic variable 
>(although it may improve readability):
>
>int f(char *x)
>{
>     /* check x, if it is a null pointer, return immediately */
>     if (x == NULL)
>	return 0; /* indicates error */
>
>     /* normal processing */
>     ...
>
>     return 1; /* indicates normal processing */
>}

"..." can mean all and nothing...

[toc] | [prev] | [next] | [standalone]

#56047

On 01/08/2015 11:21 AM, Rosario193 wrote:
> On Tue, 06 Jan 2015 21:48:02 +0100, buja <buja66@gmail.com> wrote:
>
>> On 01/04/2015 01:46 AM, S.K.R. de Jong wrote:
>>> 	I would like the opinion of people in this forum as to whether or
>>> not the following is poor style:
>>>
>>> 	int
>>> 	f(char *x)
>>> 	{
>>> 		int diagnostic = 0;
>>>
>>> 		do {
>>> 			if (x == NULL)
>>> 				break;
>>>
>>> 			...
>>>
>>> 			diagnostic = 1;
>>>
>>> 		} while(0);
>>>
>>> 		return diagnostic;
>>> 	}
>>>
>>> The same could be accomplished with goto, of course. Which kind of
>>> construct is more acceptable for dealing with error conditions: the empty
>>> while loop? goto with labels? Something else?
>>>
>>>
>>
>> Why not return immediately if x == NULL?
>> Why the loop? Is doesn't seem to be doing anything useful (unless there
>> is some iterative calculation on x).
>>
>> This doesn't need a loop or any goto's, or even the diagnostic variable
>> (although it may improve readability):
>>
>> int f(char *x)
>> {
>>      /* check x, if it is a null pointer, return immediately */
>>      if (x == NULL)
>> 	return 0; /* indicates error */
>>
>>      /* normal processing */
>>      ...
>>
>>      return 1; /* indicates normal processing */
>> }
>
> "..." can mean all and nothing...
>
"..." was in the starting post, probably some calculation on '*x'.
But as the code was not given, I can only copy the dots and assume that 
there is some "normal processing".

[toc] | [prev] | [next] | [standalone]

Page 5 of 6 — ← Prev page 1 2 3 4 [5] 6  Next page →

Back to top | Article view | comp.lang.c

csiph-web