Understanding some code

[parjit]

Hi

I'm working on a small c project. I need to read lines from stdin, but
because gets is known to be an undefined function I've been given the
code below to use instead. It works fine but I'd like to understand what
its doing and I just find it really confusing.

Can anyone explain to me what's going on here. Thanks.

#include <string.h>
char *safegets(char *buf, int sz)
{
char *r,*p;
return (buf==NULL || sz<1) ? NULL :
sz==1 ? *buf='\0', buf :
!(r=fgets(buf,sz,stdin)) ? r :
(p = strchr(buf, '\n')) ? *p=0, r : r;
}

 

[Seebs]

I'm working on a small c project. I need to read lines from stdin, but
because gets is known to be an undefined function I've been given the
code below to use instead. It works fine but I'd like to understand what
its doing and I just find it really confusing.

That is because this code is confusing.

Can anyone explain to me what's going on here. Thanks.

#include <string.h>
char *safegets(char *buf, int sz)
{
char *r,*p;
return (buf==NULL || sz<1) ? NULL :
sz==1 ? *buf='\0', buf :
!(r=fgets(buf,sz,stdin)) ? r :
(p = strchr(buf, '\n')) ? *p=0, r : r;
}

Okay, first a top-level view: Someone who thinks he's a lot smarter than
he actually is is trying to show off. I used to write code like this,
maybe fifteen or twenty years ago.

Okay, let's start by expanding this a bit:

char *safegets(char *buf, int size)
{
char *temp1, *temp2;
return (buf == NULL || size < 1) ? NULL :
size == 1 ? (*buf = '\0', buf) :
!(temp1 = fgets(buf, size, stdin)) ? temp1 :
(temp2 = strchr(buf, '\n')) ? (*temp2 = 0, temp1) :
temp1;
}

Wow, that's awful.

Quick intro:
return x ? y : z
is basically the same as
if (x) {
return y;
} else {
return z;
}

?: is like an if/then, except you use it as part of an expression,
instead of on statements. So you could do something like
abs_of_x = (x < 0) ? (-1 * x) : (x);
and that's the same effect as
if (x < 0) {
abs_of_x = (-1 * x);
} else {
abs_of_x = x;
}

Let's convert that to if/else:
char *safegets(char *buf, int size);
{
char *temp1, *temp2;
if (buf == NULL || size < 1) {
return NULL;
} else if (size == 1) {
*buf = '\0';
return buf;
} else if (!(temp1 = fgets(buf, size, stdin))) {
return temp1;
} else if (temp2 = strchr(buf, '\n')) {
*temp2 = 0;
return temp1;
} else {
return temp1;
}
}

Okay, this is still crap. I would not approve this code in a tech
review but it's getting legible. Lemme give it some comments:

/* read at most size characters into buf, resulting in a
* null-terminated string. If buf is null or size is not
* at least 1, returns null. If fgets() fails, returns
* null. Otherwise, returns buf, with the first newline
* (if any) replaced with a null byte. Since fgets() in
* theory stops with the newline, that just trims a trailing
* newline.
*/
char *safegets(char *buf, int size);
{
char *temp1, *temp2;
if (buf == NULL || size < 1) {
/* if buf is NULL, or we've said that less than
* one byte is available, return a NULL pointer;
* you give me invalid inputs, I give you invalid
* outputs.
*/
return NULL;
} else if (size == 1) {
/* if we have exactly one byte, it has to be
* the null terminator.
*/
*buf = '\0';
return buf;
} else if (!(temp1 = fgets(buf, size, stdin))) {
/* assign the results of fgets() into a
* new value named temp1. If it's "false"
* (a null pointer), return that newly
* generated null pointer.
*/
return temp1;
} else if (temp2 = strchr(buf, '\n')) {
/* if we find a newline in the string, replace
* it with a null byte. Return the pointer
* returned by fgets. Which is identical to buf,
* mind you.
*/
*temp2 = 0;
return temp1;
} else {
/* there was no newline, but we got a string,
* return it unmodified.
*/
return temp1;
}
}

Ugh. Not very consistent, but at least it makes sense.

char *safegets(char *buf, int size);
{
char *temp1, *temp2;
if (buf == NULL || size < 1) {
return NULL;
} else if (size == 1) {
*buf = '\0';
return buf;
}
temp1 = fgets(buf, size, stdin);
if (!temp1) {
/* fgets failed */
return NULL;
}
temp2 = strchr(buf, '\n');
if (temp2) {
/* found a newline, trim it */
*temp2 = '\0';
}
return buf;
}

That's a little clearer. Still, it's pretty kludgy. Let's see
if we can't make it a little friendlier. At this point, it's more
clear what the goal is: The goal is to write a wrapper around fgets()
which strips the trailing newline. For historical reasons, "gets()"
trims the terminating newline, fgets() doesn't.

Here's a first pass:

char *safegets(char *buf, int size) {
int c;
int count = 1;
char *s = buf;
if (s && size > 0) {
while ((c = getchar()) != EOF &&
c != '\n' &&
count++ < size) {
*s++ = c;
}
*s++ = '\0';
return buf;
} else {
/* invalid arguments */
return NULL;
}
}

Here's my test program:

#include <stdio.h>

char *safegets(char *buf, int size) {
int c;
int count = 1;
char *s = buf;
if (s && size > 0) {
while ((c = getchar()) != EOF &&
c != '\n' &&
count++ < size) {
*s++ = c;
}
*s++ = '\0';
return buf;
} else {
/* invalid arguments */
return NULL;
}
}

int
main(void) {
char buf[10] = { "xxxxxxxxx" };
int i;

printf("doing safegets, length 6:\n");
safegets(buf, 6);
for (i = 0; i < 8; ++i) {
printf("\t[%d]: 0x%02x [%c]\n",
i,
(unsigned char) buf,
isprint((unsigned char) buf) ?
buf : '.');
}
printf("[5] or earlier should be 0x00 [.], following should be [x].\n");
return 0;
}

This passed the obvious edge cases:
* values less than 5 characters long
* exactly 5 characters plus a newline
* newline with following characters
* more than 6 characters

-s

 

[Ian Collins]

Hi

I'm working on a small c project. I need to read lines from stdin, but
because gets is known to be an undefined function I've been given the
code below to use instead. It works fine but I'd like to understand what
its doing and I just find it really confusing.

Can anyone explain to me what's going on here. Thanks.

#include<string.h>
char *safegets(char *buf, int sz)
{
char *r,*p;
return (buf==NULL || sz<1) ? NULL :
sz==1 ? *buf='\0', buf :
!(r=fgets(buf,sz,stdin)) ? r :
(p = strchr(buf, '\n')) ? *p=0, r : r;
}

If the project contains code like this, run away!

 

[Paul N]

Hi

I'm working on a small c project. I need to read lines from stdin, but
because gets is known to be an undefined function I've been given the
code below to use instead. It works fine but I'd like to understand what
its doing and I just find it really confusing.

Can anyone explain to me what's going on here. Thanks.

#include <string.h>
char *safegets(char *buf, int sz)
{
   char *r,*p;
   return (buf==NULL || sz<1) ? NULL :
     sz==1 ? *buf='\0', buf :
     !(r=fgets(buf,sz,stdin)) ? r :
     (p = strchr(buf, '\n')) ? *p=0, r : r;
}

I think whoever wrote that code may be messing you about.

If you say something like

return a ? b : c;

then this is the same as:

if (a) return b; else return c;

and because "return" causes the following statements to be missed out,
this is the same as:

if (a) return b;
return c;

So applying that a few times and making a few other small changes, we
get:

char *safegets(char *buf, int sz)
{
char *r,*p;
if (buf==NULL || sz<1) return NULL;
if (sz==1) { buf[0]='\0'; return buf; }
r=fgets(buf,sz,stdin);
if (!r) return r;
p = strchr(buf, '\n');
if (p) { p[0]=0; return r; }
return r;
}

which in turn means:
bail out if buf is NULL or sz (size) is too small
return empty string if there's no space for anything longer
read in one line, or sz-1 characters, whichever is shorter
return NULL if that failed
look for a newline
replace it if you find one
return the string read (same as buf)

Hope that helps.
Paul.

 

[Uno]

Okay, first a top-level view: Someone who thinks he's a lot smarter than
he actually is is trying to show off. I used to write code like this,
maybe fifteen or twenty years ago.

Okay, let's start by expanding this a bit:

Seebs gives himself to be this person who understands the learning
process, indeed can even divine what others are learning.

And, here, he's answering somebody's homework. This is the first
worksheet the prof hands out.

Way to expand The Topic. How about if we call c.l.c. Seebs' blog now?

 

[John Kelly]

Seebs gives himself to be this person who understands the learning
process, indeed can even divine what others are learning.

And, here, he's answering somebody's homework. This is the first
worksheet the prof hands out.

Way to expand The Topic. How about if we call c.l.c. Seebs' blog now?

There's no business like show business.

 

[Seebs]

And, here, he's answering somebody's homework. This is the first
worksheet the prof hands out.

Is it? What university, or course? We could, of course verify this. I
didn't see anything obviously marking it as homework, and I have a really
hard time imagining that being the first of ANYTHING in a C course, because
you generally get through a lot of other stuff before you start working
with nested ?:.

-s

 

[Eric Sosman]

Is it? What university, or course? We could, of course verify this. I
didn't see anything obviously marking it as homework, and I have a really
hard time imagining that being the first of ANYTHING in a C course, because
you generally get through a lot of other stuff before you start working
with nested ?:.

... and especially with nested ?: used stupidly. The code as
shown won't compile, and even with the obvious fix it won't survive
code review. It's on a par with `if((!isspace(ch)!=1) == 0)'.

There are two audiences for source code: Compilers and programmers.
The latter is by far the more important; cater to their needs before
worrying about those of the former.

 

[Seebs]

... and especially with nested ?: used stupidly. The code as
shown won't compile, and even with the obvious fix it won't survive
code review. It's on a par with `if((!isspace(ch)!=1) == 0)'.

It's really bad. And the interesting thing is, despite Uno's confident
assertion that this is the "first worksheet", I can find no instances
of that code anywhere except this thread.

But mostly, it's just plain too advanced to be a first handout, but
it's very plausible as a cargo cult thing someone would hand you
and tell you it's great. I will say, though. If you look at some
of the code we see in this newsgroup, hypothesizing a teacher who
would hand that out has substantial explanatory power.

-s

 

[Uno]

It's really bad. And the interesting thing is, despite Uno's confident
assertion that this is the "first worksheet", I can find no instances
of that code anywhere except this thread.

But mostly, it's just plain too advanced to be a first handout, but
it's very plausible as a cargo cult thing someone would hand you
and tell you it's great. I will say, though. If you look at some
of the code we see in this newsgroup, hypothesizing a teacher who
would hand that out has substantial explanatory power.

But you certainly don't think this occured in production code, and this
is the first week of class for people taking computer programming in a
lot of places.

If that place is India, this might be high school stuff.

 

[Willem]

Seebs wrote:
) <big snip>
) Ugh. Not very consistent, but at least it makes sense.
)
) char *safegets(char *buf, int size);
) {
) char *temp1, *temp2;
) if (buf == NULL || size < 1) {
) return NULL;
) } else if (size == 1) {
) *buf = '\0';
) return buf;
) }
) temp1 = fgets(buf, size, stdin);
) if (!temp1) {
) /* fgets failed */
) return NULL;
) }
) temp2 = strchr(buf, '\n');
) if (temp2) {
) /* found a newline, trim it */
) *temp2 = '\0';
) }
) return buf;
) }
)
) That's a little clearer. Still, it's pretty kludgy. Let's see
) if we can't make it a little friendlier. At this point, it's more
) clear what the goal is: The goal is to write a wrapper around fgets()
) which strips the trailing newline. For historical reasons, "gets()"
) trims the terminating newline, fgets() doesn't.
)
) Here's a first pass:
)
) char *safegets(char *buf, int size) {
) int c;
) int count = 1;
) char *s = buf;
) if (s && size > 0) {
) while ((c = getchar()) != EOF &&
) c != '\n' &&
) count++ < size) {
) *s++ = c;
) }
) *s++ = '\0';
) return buf;
) } else {
) /* invalid arguments */
) return NULL;
) }
) }

Now why in blazes did you suddenly decide to drop the fgets() ?

With a little massaging, the fgets() version could be something like:

char *safegets(char *buf, int size);
{
char *nl;
if (buf == NULL || size < 1) {
return NULL;
}
if (!fgets(buf, size, stdin)) {
/* fgets failed */
return NULL;
}
nl = strchr(buf, '\n');
if (nl) {
/* found a newline, trim it */
*nl = '\0';
}
return buf;
}

Which looks clean as a whistle, and nicely separates
the three steps the function is supposed to take:
- sanity check inputs
- read line
- trim newline
- return result.
Four. The four steps the function is supposed to take. Ahem.


SaSW, Willem
--
Disclaimer: I am in no way responsible for any of the statements
made in the above text. For all I know I might be
drugged or something..
No I'm not paranoid. You all think I'm paranoid, don't you !
#EOT

 

[Seebs]

Now why in blazes did you suddenly decide to drop the fgets() ?

Because, IMHO, it's the wrong tool for the job -- specifically, it
gives you no way to trim the trailing null without iterating over
the entire string an extra time. It seemed to me that avoiding
that would be an improvement, although obviously it's a debatable
point.

Which looks clean as a whistle, and nicely separates
the three steps the function is supposed to take:
- sanity check inputs
- read line
- trim newline
- return result.
Four. The four steps the function is supposed to take. Ahem.

AMONG the steps this function is supposed to take...

-s

 

[Nick Keighley]

Hi

I'm working on a small c project. I need to read lines from stdin, but
because gets is known to be an undefined function I've been given the
code below to use instead. It works fine but I'd like to understand what
its doing and I just find it really confusing.

Can anyone explain to me what's going on here. Thanks.

#include <string.h>
char *safegets(char *buf, int sz)
{
   char *r,*p;
   return (buf==NULL || sz<1) ? NULL :
     sz==1 ? *buf='\0', buf :
     !(r=fgets(buf,sz,stdin)) ? r :
     (p = strchr(buf, '\n')) ? *p=0, r : r;
}

I think whoever wrote that code may be messing you about.

If you say something like

return a ? b : c;

then this is the same as:

if (a) return b; else return c;

and because "return" causes the following statements to be missed out,
this is the same as:

if (a) return b;
return c;

So applying that a few times and making a few other small changes, we
get:

char *safegets(char *buf, int sz)
{
char *r,*p;
if (buf==NULL || sz<1) return NULL;
if (sz==1)  { buf[0]='\0'; return buf; }
r=fgets(buf,sz,stdin);
if (!r) return r;
p = strchr(buf, '\n');
if (p) { p[0]=0; return r; }
return r;

}

which in turn means:
bail out if buf is NULL or sz (size) is too small
return empty string if there's no space for anything longer
read in one line, or sz-1 characters, whichever is shorter
return NULL if that failed
look for a newline
replace it if you find one
return the string read (same as buf)

Hope that helps.

is you space bar broken?

 

[Jeff Clough]

Is it? What university, or course? We could, of course verify this. I
didn't see anything obviously marking it as homework, and I have a really
hard time imagining that being the first of ANYTHING in a C course, because
you generally get through a lot of other stuff before you start working
with nested ?:.

For what it's worth, I've seen exercises along these lines where the
intro is basically "You've been assigned to fix a bug in some maniac's
code." But something like that in the first week? Ehhhh...

Jeff

 

[Ben Bacarisse]

On 08/25/10 08:51 AM, parjit wrote:

If the project contains code like this, run away!

Actually I don't find the idea (a big nested conditional) as awful as so
many other people seem to do. I hate the layout (indentation and lack of
spaces) and it has some unnecessary complications (r, for example, and a
pointless special case when sz == 1) but a nested conditional is not,
itself, a monstrosity to me. Maybe too many years of Lisp perhaps...

It's a shame that history has left us with strchr that returns NULL on
failure. If it returned a pointer to the terminating null, then one
could write *strchr(buf, '\n') = 0 (and that's not the only
simplification it would yield). Still, there is always strcspn!.

In the end, only one level of conditional is required:

char *safegets(char *buf, int sz)
{
return !buf || sz < 1 || !fgets(buf, sz, stdin)
? NULL
: (buf[strcspn(buf, "\n")] = 0, buf);
}

I think the result is reasonably clear. There are three simple reasons
to return NULL. In all other cases the buffer pointer is returned after
possibly replacing a newline with null.

Of course, the curiosity is that I'd never write this. I can't imagine
any project where the coding standard would consider this to be OK. The
fact that I do is just a quirk.

 

[Seebs]

Actually I don't find the idea (a big nested conditional) as awful as so
many other people seem to do.

I don't find a big nested conditional utterly horrible.

I do find the (*foo = bar, foo) type sub-expressions unacceptable.

Too much conceptual nesting. I do not think conditional expressions should
have conditional side-effects, that's just too error-prone.

-s

 

[parjit]

Is it? What university, or course? We could, of course verify this. I
didn't see anything obviously marking it as homework, and I have a
really hard time imagining that being the first of ANYTHING in a C
course, because you generally get through a lot of other stuff before
you start working with nested ?:.

It is not a homework, it is code the teacher gave us to use as a
component in our homeworks to take care of error checking and unsafeness
in standard functions. We do not have to explain this code but I want to
understand it for my own interest. It is very complicated code, I don't
think I will ever be able to write code this clever

Here are the safe versions of malloc and free. Also confusing to me!

void *safemalloc(int sz)
{
void *r;
return sz<=0 ? 0 :
(r=malloc(sz)) ? r :
((!!printf("fatal error cannot alocate %d bytes dumping core",sz)
&& (exit(1),0)),NULL);
}

void safefree(void *p)
{
free(p?p:
(!!printf("fatal error cannot free null pointer dumping core")
&& (exit(1),0),NULL));
}

 

[Paul N]

I think whoever wrote that code may be messing you about.

If you say something like

return a ? b : c;

then this is the same as:

if (a) return b; else return c;

and because "return" causes the following statements to be missed out,
this is the same as:

if (a) return b;
return c;

So applying that a few times and making a few other small changes, we
get:

char *safegets(char *buf, int sz)
{
char *r,*p;
if (buf==NULL || sz<1) return NULL;
if (sz==1)  { buf[0]='\0'; return buf; }
r=fgets(buf,sz,stdin);
if (!r) return r;
p = strchr(buf, '\n');
if (p) { p[0]=0; return r; }
return r;

}

which in turn means:
bail out if buf is NULL or sz (size) is too small
return empty string if there's no space for anything longer
read in one line, or sz-1 characters, whichever is shorter
return NULL if that failed
look for a newline
replace it if you find one
return the string read (same as buf)

Hope that helps.

is you space bar broken?

Er, no. I was trying to re-organise the original code to make it
easier to understand. It didn't occur to me to go through it putting
spaces in, to be honest, though code I had written myself would have
more spaces. Even looking back, I think the lack of spaces is not
really the code's biggest problem.

AFAIK, the only problem with my system is the way blank lines appear
before the last } of all my functions. See above, for instance. I
think this is due to a problem with Google's quoting mechanism.

Paul.

 

[Jorgen Grahn]

It is not a homework, it is code the teacher gave us to use as a
component in our homeworks to take care of error checking and unsafeness
in standard functions. We do not have to explain this code but I want to
understand it for my own interest. It is very complicated code, I don't
think I will ever be able to write code this clever

The code may have been clever, but it was *not* clever to give it to
people without documentation. It's easy to explain, in a few
sentences, what a gets() replacement does, and if the author has done
that well I don't feel any need to review the code, even if it looks
too clever. If he hasn't, I do not trust his judgement, and not his
code either.

Here are the safe versions of malloc and free. Also confusing to me!

void *safemalloc(int sz)
{
void *r;
return sz<=0 ? 0 :
(r=malloc(sz)) ? r :
((!!printf("fatal error cannot alocate %d bytes dumping core",sz)
&& (exit(1),0)),NULL);
}

void safefree(void *p)
{
free(p?p:
(!!printf("fatal error cannot free null pointer dumping core")
&& (exit(1),0),NULL));
}

There are so many problems with that, I don't know where to start.
I half suspect you are joking ... but anyway, to name just a few that
I see:

- no documentation (unless you omitted it)
- he says "safe" but means "the program will exit", potentially
losing data, killing people, or whatever. It's generally
*less* safe than to allow the caller to handle the error.
- ... and here the caller will have to handle some errors anyway
(why exit for out-of-memory but not for safemalloc(-42)?)
- exit(1) will never cause a core dump, not even on systems which
support core dumps
- int is the wrong argument to a malloc(); it should be size_t
- free(0) is safe (and useful!), but safefree(0) is not.
- lots of the weird subexpressions (e.g. !!x) seem useless to me,
but I haven't bothered to decode them.

I think you should continue the course, but not trust what the teacher
says. You have internet access, so you can study good C code, do
exercises and read this group between classes.

/Jorgen

 

[Seebs]

It is not a homework, it is code the teacher gave us to use as a
component in our homeworks to take care of error checking and unsafeness
in standard functions. We do not have to explain this code but I want to
understand it for my own interest. It is very complicated code, I don't
think I will ever be able to write code this clever

Ugh.

No offense, but your teacher sucks at teaching C.

Here are the safe versions of malloc and free. Also confusing to me!

These are the same kind of trickery.

void *safemalloc(int sz)
{
void *r;
return sz<=0 ? 0 :
(r=malloc(sz)) ? r :
((!!printf("fatal error cannot alocate %d bytes dumping core",sz)
&& (exit(1),0)),NULL);
}

I would refuse to pay money to learn C from someone who wrote this. I count
three obvious errors plus a bewildering variety of crappy design choices and
badly-written code.

void safefree(void *p)
{
free(p?p:
(!!printf("fatal error cannot free null pointer dumping core")
&& (exit(1),0),NULL));
}

Same here, plus it's actually less safe than the standard C one, because
free(NULL) has been well-defined for something over twenty years.

I wish you the best of luck in your course. Know that you will be learning
C despite this idiot, not because of him.

-s