Comments on my code?

[Platonic Solid]

Hi-

I am learning C from some old lecture notes, but they don't have
solutions so I'd like some feedback on my code if anyone has the time.

This is an exercise: "Write a program to trim any leading whitespace
from a string and return a newly allocated buffer containing the trimmed
string. Don't forget to handle errors."

main(argc, argv)
int argc; char **argv;
{
char *strtrim(), *r=0;
puts(argc>1 && (r=strtrim(*++a rgv)) ? r : "Unspecifie d error");
free(r);
}

/* trims initial whitespace */
char *strtrim(s)
char *s;
{
char *r, *malloc();
for( ; isspace(*s); s++);
r=malloc(strlen (s)+1);
if(r)
strcpy(r,s);
return r;
}

This looks OK to me and works correctly, but the compiler produces some
mysterious warnings about conflicting definitions that I don't really
understand.
--
How come we never talk anymore?

[jacob navia]

Platonic Solid wrote:

Hi-

I am learning C from some old lecture notes, but they don't have
solutions so I'd like some feedback on my code if anyone has the time.

This is an exercise: "Write a program to trim any leading whitespace
from a string and return a newly allocated buffer containing the trimmed
string. Don't forget to handle errors."

main(argc, argv)
int argc; char **argv;
{
char *strtrim(), *r=0;
puts(argc>1 && (r=strtrim(*++a rgv)) ? r : "Unspecifie d error");
free(r);
}

/* trims initial whitespace */
char *strtrim(s)
char *s;
{
char *r, *malloc();
for( ; isspace(*s); s++);
r=malloc(strlen (s)+1);
if(r)
strcpy(r,s);
return r;
}

This looks OK to me and works correctly, but the compiler produces some
mysterious warnings about conflicting definitions that I don't really
understand.
--
How come we never talk anymore?

You should not declare malloc like that.
#include <stdlib.h>
and never define your own definitions for library functions.

You need
#include <string.h>
for strlen's prototype.

And:

char *strtrim(s)
char *s;

This is old fashioned C. Better is:

char *strtrim(char *s)

P.S. What happens when s is NULL?

jacob

[Harald van =?UTF-8?B?RMSzaw==?=]

Platonic Solid wrote:

Hi-

I am learning C from some old lecture notes, but they don't have
solutions so I'd like some feedback on my code if anyone has the time.

This is an exercise: "Write a program to trim any leading whitespace
from a string and return a newly allocated buffer containing the trimmed
string. Don't forget to handle errors."

main(argc, argv)
int argc; char **argv;
{
char *strtrim(), *r=0;
puts(argc>1 && (r=strtrim(*++a rgv)) ? r : "Unspecifie d error");
free(r);
}

/* trims initial whitespace */
char *strtrim(s)
char *s;
{
char *r, *malloc();
for( ; isspace(*s); s++);
r=malloc(strlen (s)+1);
if(r)
strcpy(r,s);
return r;
}

This looks OK to me and works correctly, but the compiler produces some
mysterious warnings about conflicting definitions that I don't really
understand.

The program you wrote would be a correct way to write code a long time ago,
so if your lecture notes were really old, then well done. It's not a right
way to do things now, though. Try this version, and see if you understand
the changes I made:

#include <stdio.h /* include the headers that declare the library */
#include <stdlib.h/* functions and objects you are using. you should */
#include <string.h/* not declare them in your own program, especially*/
#include <ctype.h /* the wrong way; that is what caused your warning */

/*
* a forward declaration for your function should not be local to
* another function. also, you can specify the types of the parameters
* in the declaration as well as the definition. doing so allows the
* compiler to check and automatically convert the values you will be
* passing to the function.
*
* I renamed the function because technically, strtrim isn't a valid name
* for it, but the reasons why probably won't be of interest to you at
* this stage.
*/
char *trimstr(char *);

/*
* the types of the parameters should be specified in the parameter list,
* not between the ) and {. it's still valid, but has the same problems
* as not specifying the parameter types in the function declaration.
* and you should specify the return type, even if it's int.
*/
int main(int argc, char **argv) {
/*
* for readability reasons, I would recommend against your use of ?:,
* and use if statements instead.
*/
if (argc 1) {
char *r = trimstr(argv[1]);
if (!r) {
/* error messages should normally go to stderr, not stdout */
fputs("Unspecif ied error\n", stderr);
} else {
puts(r);
free(r);
}
}
/* main returns int (even in your original version), so return an int */
return 0;
}

char *trimstr(char *s) {
char *r;
/* for readability, you can rewrite your for loop */
while (isspace(*s))
s++;
/*
* malloc doesn't need to be declared manually here, with <stdlib.h>
* included. and malloc doesn't return 'char *' anymore, but 'void *'
* void is probably not covered in your lecture notes, but in the
* context of pointers, it means a pointer to any object, but you
* don't have the object's type.
*/
r = malloc(strlen(s )+1);
if(r)
strcpy(r,s);
return r;
}

[Platonic Solid]

On 1 Sep 2007 at 12:53, jacob navia wrote:

P.S. What happens when s is NULL?

If you look carefully I check argc>1, so it never gets called with s
null.

--
How come we never talk anymore?

[Army1987]

On Sat, 01 Sep 2007 14:01:58 +0200, Platonic Solid wrote:

Hi-

I am learning C from some old lecture notes, but they don't have
solutions so I'd like some feedback on my code if anyone has the time.

This is an exercise: "Write a program to trim any leading whitespace
from a string and return a newly allocated buffer containing the trimmed
string. Don't forget to handle errors."

main(argc, argv)
int argc; char **argv;

In modern C this is usually written as
int main(int argc, char *argv[])
But the old form continues to work, even it "looks strange".

{
char *strtrim(), *r=0;

Better put function declaration at file scope, and use NULL
defined in <stddef.hand other headers instead of 0. Of course
your form will work, too, but it is "ugly".

puts(argc>1 && (r=strtrim(*++a rgv)) ? r : "Unspecifie d error");

To somebody which doesn't remember all the precedence rules by
heart, this could be confusing. If you *really* think that if/else
is evil (it isn't, at least it'd allow you to write the error
message to stderr), at least put parentheses around the first
operand of ?: ...
<nitpick pedantry="high" >Nowadays puts() takes a const char *,
but you're passing it a char *, because there is no prototype in
scope.</nitpickEven if it is overwhelmingly likely to work,
usually one use the prototype for puts() found in <stdio.h>.

free(r);
}

/* trims initial whitespace */
char *strtrim(s)
char *s;
{
char *r, *malloc();

In modern C malloc returns a void*, not a char*. Also the argument
is a size_t, see FAQ 7.15.

for( ; isspace(*s); s++);
r=malloc(strlen (s)+1);

strlen returns a size_t, but here you're declaring it as returning
an int.

if(r)
strcpy(r,s);
return r;
}

This looks OK to me and works correctly, but the compiler produces some
mysterious warnings about conflicting definitions that I don't really
understand.

See FAQ 11.29a for compatibility problem between old and new C.
--
Army1987 (Replace "NOSPAM" with "email")
No-one ever won a game by resigning. -- S. Tartakower

[jacob navia]

Platonic Solid wrote:

On 1 Sep 2007 at 12:53, jacob navia wrote:

>P.S. What happens when s is NULL?

If you look carefully I check argc>1, so it never gets called with s
null.

--
How come we never talk anymore?

Sure, but is strtrim only for this particular call?
In that case a function is not necessary!

[Peter J. Holzer]

On 2007-09-01 13:26, jacob navia <ja***@jacob.re mcomp.frwrote:

Platonic Solid wrote:

>On 1 Sep 2007 at 12:53, jacob navia wrote:

>>P.S. What happens when s is NULL?

If you look carefully I check argc>1, so it never gets called with s
null.

Sure, but is strtrim only for this particular call?
In that case a function is not necessary!

What happens when you call strlen(NULL)?

hp
--
_ | Peter J. Holzer | I know I'd be respectful of a pirate
|_|_) | Sysadmin WSR | with an emu on his shoulder.
| | | hj*@hjp.at |
__/ | http://www.hjp.at/ | -- Sam in "Freefall"

[Peter J. Holzer]

On 2007-09-01 12:59, Harald van Dijk <tr*****@gmail. comwrote:

Platonic Solid wrote:

>I am learning C from some old lecture notes, but they don't have
solutions so I'd like some feedback on my code if anyone has the time.

[ lots of good advice snipped]

char *trimstr(char *s) {
char *r;
/* for readability, you can rewrite your for loop */
while (isspace(*s))
s++;

isspace expects an argument in the range [EOF, 0 .. UCHAR_MAX]. But
*s will be in the range [CHAR_MIN .. CHAR_MAX], so isspace may be called
with an illegal value, resulting in undefined behaviour.

You must get the value into the correct range first:

while (isspace((unsig ned char)*s))
s++;

hp

--
_ | Peter J. Holzer | I know I'd be respectful of a pirate
|_|_) | Sysadmin WSR | with an emu on his shoulder.
| | | hj*@hjp.at |
__/ | http://www.hjp.at/ | -- Sam in "Freefall"

[Bart van Ingen Schenau]

Platonic Solid wrote:

Hi-

I am learning C from some old lecture notes, but they don't have
solutions so I'd like some feedback on my code if anyone has the time.

Judging from your code, these are extremely old lecture notes. If there
is a date on them, I would guess it to be somewhere in the 1980's.
Since that time, C has evolved a bit. Not too much but enough that it
would be worthwhile to have a more recent book to accompany these
lecture notes.
I would suggest you get a copy of "The C Programming Language", second
edition, by Kernighan and Ritchie (often referred to as K&R2)

>
This is an exercise: "Write a program to trim any leading whitespace
from a string and return a newly allocated buffer containing the
trimmed string. Don't forget to handle errors."

To use library functions, you should #include the appropriate headers.
For this program, you need

#include <stdio.h/* for puts() */
#include <stdlib.h/* for malloc()/free() */
#include <ctype.h/* for isspace() */
#include <string.h/* for strcpy() */

main(argc, argv)
int argc; char **argv;

This style of declaring parameters is considered to be outdated.
It is preferred that you write it now like this:
int main(int argc, char **argv)

{
char *strtrim(), *r=0;

It is not wrong to declare a function at this point, but it is not
considered to be good style.
It is recommended that you declare functions at file scope, and that you
give a prototype for the function.
The prototype for strtrim() looks like
char *strtrim(char*) ;
The advantage of a prototype is that the compiler can verify (and
complain!) that you call the function in the correct way.

As a more personal style issue, I prefer to have one item per
declaration. I would have written the above declaration as:
char *strtrim(char*) ;
char *r;
I also might have used a more descriptive name for r.

puts(argc>1 && (r=strtrim(*++a rgv)) ? r : "Unspecifie d error");

C allows you to write extremely complex expressions, but usually it is
best to avoid using that freedom. (exception: IOCCC entries)
The statement above is right at the limit of how complex you should make
it (and some would argue it is over the limit).

free(r);

As main() returns a success/failure code to the OS, you should make sure
that it is a sensible code:
return 0; /* indicates success. Other code are EXIT_SUCCESS and
EXIT_FAILURE */

}

/* trims initial whitespace */
char *strtrim(s)
char *s;

Same comment as with main(): you should place the argument type also
within the parentheses.
char *strtrim(char *s)

{
char *r, *malloc();

You should not write a declaration for library functions, like malloc().
Instead you should #include the required header (see above).

for( ; isspace(*s); s++);
r=malloc(strlen (s)+1);
if(r)
strcpy(r,s);
return r;
}

This looks OK to me and works correctly, but the compiler produces
some mysterious warnings about conflicting definitions that I don't
really understand.

Those warnings are probably due to the required headers that you forgot
to include.
Modern compilers will complain if the encounter a call to a function
that they have not yet seen before (like your calls to puts, isspace
and strcpy). The complaints are because the compiler has to make some
assumptions that are likely to be wrong.

BTW, if you have questions about compiler messages, it works so much
better if you post the compiler output (along with the code you fed
into the compiler). Most of us will know immediately what it means,
because we have seen the message before.

Bart v Ingen Schenau
--
a.c.l.l.c-c++ FAQ: http://www.comeaucomputing.com/learn/faq
c.l.c FAQ: http://www.eskimo.com/~scs/C-faq/top.html
c.l.c++ FAQ: http://www.parashift.com/c++-faq-lite/

[Harald van =?UTF-8?B?RMSzaw==?=]

Peter J. Holzer wrote:

On 2007-09-01 12:59, Harald van Dijk <tr*****@gmail. comwrote:

>Platonic Solid wrote:

>>I am learning C from some old lecture notes, but they don't have
solutions so I'd like some feedback on my code if anyone has the time.

[ lots of good advice snipped]

>char *trimstr(char *s) {
char *r;
/* for readability, you can rewrite your for loop */
while (isspace(*s))
s++;

isspace expects an argument in the range [EOF, 0 .. UCHAR_MAX]. But
*s will be in the range [CHAR_MIN .. CHAR_MAX], so isspace may be called
with an illegal value, resulting in undefined behaviour.

You must get the value into the correct range first:

while (isspace((unsig ned char)*s))
s++;

Thanks, I forgot about that. It would be nice if I could get my system to
warn about passing a char; it's not the first time I've left out the cast.