Declaration specifiers

[cp]

I'm not an excellent C-programmer so forgive possible, in-your-eyes, stupid
mistakes below.

struct book {
char name[20];
char genre[20];
int serial_number;
}

struct book * insert_book(char bookname[20], char bookgenre[20], int
booknumber) {
struct book * book_ptr;
book_ptr = malloc(sizeof(struct book));
if(book_ptr == NULL){
printf("Memory not allocated\n");
exit(-1);
}
(*book_ptr).name[20] = bookname[20];
(*book_ptr).genre[20] = bookgenre[20];
(*book_ptr).serial_number = booknumber;
printf("Completed\n");
return(book_ptr);
}

int main(void){
struct book * insert_book_ptr;
insert_book_ptr = insert_book("Hello World", "horror", 123);
printf("bookname is %s, genre is %s and number is
%d\n",(*insert_book_ptr).name, (*insert_book_ptr).genre,
(*insert_book_ptr).serial_number);
}

When I compile it (gcc 4.03) i am informed that:
c:11: Error: "two or more data types in declaration specifiers"

Line 11 is: struct book * insert_book(char bookname[20], char bookgenre[20],
int booknumber) {

What am i doing wrong?

 

[spibou]

I'm not an excellent C-programmer so forgive possible, in-your-eyes, stupid
mistakes below.

struct book {
char name[20];
char genre[20];
int serial_number;
}

Add a ; here.

 

[narasimaperumal]

struct book {
char name[20];
char genre[20];
int serial_number;
};<======U have missed the semi-colon (

 

[cp]

Add a ; here.

Thank you

 

[Eric Sosman]

I'm not an excellent C-programmer so forgive possible, in-your-eyes, stupid
mistakes below.

struct book {
char name[20];
char genre[20];
int serial_number;
}

Others have noted the missing semicolon here. But
read on anyhow ...

struct book * insert_book(char bookname[20], char bookgenre[20], int
booknumber) {
struct book * book_ptr;
book_ptr = malloc(sizeof(struct book));

Small improvement possible: Write

book_ptr = malloc(sizeof *book_ptr);

instead. (The reasons have been discussed often; search
groups.google.com to learn about them.)

if(book_ptr == NULL){
printf("Memory not allocated\n");
exit(-1);

Quibble: EXIT_FAILURE would be a better choice than -1
here. It's defined in <stdlib.h> (which you've already
included, right?) as an appropriate "program failed" status
value for whatever system you're using. (I have personal
experience of a system where -1 would indicate success, not
failure.)

}
(*book_ptr).name[20] = bookname[20];
(*book_ptr).genre[20] = bookgenre[20];

Here's the important one: These assignments don't do
what you think they do. In fact, each fetches a single
"off the end" character from its source array and stores
it "off the end" of the target array. (Or tries, to; once
you start rummaging around outside the bounds of the arrays,
there's really no telling what might happen.) Read up on
the strcpy() function. And for safety's sake, read up on
strlen() as well.

(*book_ptr).serial_number = booknumber;

Informational: Instead of `(*book_ptr).something', you
can use `book_ptr->something'. It means the same thing, but
is more idiomatic. It is also easier to read when the struct
itself contains pointers that you want to dereference:

book_ptr->next_book->serial_number = 42;
(*(*book_ptr).next_book)).serial_number = 42;

printf("Completed\n");
return(book_ptr);
}

int main(void){
struct book * insert_book_ptr;
insert_book_ptr = insert_book("Hello World", "horror", 123);
printf("bookname is %s, genre is %s and number is
%d\n",(*insert_book_ptr).name, (*insert_book_ptr).genre,
(*insert_book_ptr).serial_number);

Either `return 0;' or `return EXIT_SUCCESS;' would be
a good addition here.

 

[Simon Biber]

I'm not an excellent C-programmer so forgive possible, in-your-eyes, stupid
mistakes below.

struct book {
char name[20];
char genre[20];
int serial_number;
}

All good so far. But you need a semicolon here, to terminate the
definition of the struct book type.

struct book * insert_book(char bookname[20], char bookgenre[20], int
booknumber) {

Your use of array brackets here, and in particular the literal 20,
suggests to me that you don't understand how arrays are passed to
functions. The compiler completely ignores the numbers that you put in
this prototype. It actually generates code that takes a pointer to char
for each of the bookname and bookgenre arguments.

To show that you know what is actually happening, I'd recommend you
write it more literally:
struct book * insert_book(
char *bookname, char *bookgenre, int booknumber) {

However, since the function below should not attempt to modify the
contents of the bookname or bookgenre strings, it would make sense to
reflect that in the types of the arguments too:
struct book * insert_book(
const char *bookname, const char *bookgenre, int booknumber) {

struct book * book_ptr;
book_ptr = malloc(sizeof(struct book));

The above is fine, but many people prefer to use the sizeof operator on
the object itself rather than the type:
book_ptr = malloc(sizeof *book_ptr);
That way, if the type changes in the future, there is one less place you
need to change it.

if(book_ptr == NULL){
printf("Memory not allocated\n");
exit(-1);

It's nice to see you did check for failure of malloc. However, the
meaning of -1 as an exit code is not well defined. It may indicate
success on one machine and failure on another. It may even do something
weird and unexpected. The better thing to do is exit(EXIT_FAILURE);
which should indicate that the program failed.

}
(*book_ptr).name[20] = bookname[20];

The name array has only 20 elements. Here you attempt to write to the
21st element. This is a BAD thing.

What you really want to do is copy the string from bookname to name.
That's a tedious thing to do manually; you need a loop to copy
characters one at a time until the null character is reached.
Thankfully there is a function provided in C that can do it for you:
strcpy().

strcpy(book_ptr->name, bookname);

If you do use strcpy, be very sure that bookname doesn't exceed 19
characters (19 characters plus 1 for the null character makes 20, the
size of the name array).

if(strlen(bookname) >= sizeof book_ptr->name)
{
printf("book name too long\n");
exit(EXIT_FAILURE);
}
else
{
strcpy(book_ptr->name, bookname);
}

(*book_ptr).genre[20] = bookgenre[20];

You need the same thing here.

(*book_ptr).serial_number = booknumber;
printf("Completed\n");
return(book_ptr);

return is not a function. It doesn't need brackets.
Just do:
return book_ptr;

 

[cp]

Thank you all for your input. It was greatly appreciated.