Malloc, Structure Help

[Mannequin*]

Hi all,
I'm working on a quick program to bring the Bible into memory from a
text file. Anyway, I have three questions to ask.

First, is my implementation of malloc () correct in the program to
follow?

Second, have I correctly passed the structure's pointer to the
functions in this program?

Last, how do I go about initializing the structure variables in the
given function?

Here's the program:

*----------------------------------------*

#include <stdio.h>
#include <stdlib.h>

struct bibtxt
{
char book[32];
int chapter;
int verse;
char text[500];
};

typedef struct bibtxt BIBLE;

void fillverse (BIBLE *pverse);

int main (void)
{
BIBLE *pverse;
int x;

pverse = malloc (1000 * sizeof (BIBLE));
if (pverse == NULL)
{
printf ("It didn't work.\n");
return -1;
}

printf ("pverse = %d\n", (sizeof (BIBLE) * 1000));

for (x = 0; x < 1000; x++)
fillverse (pverse);

for (x = 0; x < 1000; x++)
printverse (pverse);

free (pverse);

return 0;
}

void fillverse (BIBLE *pverse)
{
int x, y;

for (x = 0; x < 1000; x++)
for (y = 0; y < 500; y++)
{
printf ("%s\n", pverse.text);
pverse.text[y] = '\0';
}
}

*----------------------------------------*

I know that in the function fillverse, that I have the pverse.text[y]
wrong. I've been playing with this for several hours trying to find the
right way to do what I'm wanting to do with this.

Please give my any advice on style or whatever you think may be
appropriate.

Thanks!
-M.

 

[Leor Zolman]

Hi all,
I'm working on a quick program to bring the Bible into memory from a
text file. Anyway, I have three questions to ask.

First, is my implementation of malloc () correct in the program to
follow?

Second, have I correctly passed the structure's pointer to the
functions in this program?

Last, how do I go about initializing the structure variables in the
given function?

Rather than answering the questions directly, allow me to comment on the
code en passant. You've made some design decisions here that you may want
to reconsider.

Here's the program:

*----------------------------------------*

#include <stdio.h>
#include <stdlib.h>

struct bibtxt
{
char book[32];
int chapter;
int verse;
char text[500];

Right here, it looks like the text array is supposed to represent a verse.
Rather than having a "worst case" fixed-size, you're far better off making
this a pointer-to-char and handling the allocation dynamically. The extra
complexity this would require is more than made up for by the overall
increase in memory efficiency. I won't show the entirety of the changes
this would entail, but I'll try to hit the highlights. For starters, change
the above to:
char *text;

};

typedef struct bibtxt BIBLE;

void fillverse (BIBLE *pverse);

int main (void)
{
BIBLE *pverse;
int x;

pverse = malloc (1000 * sizeof (BIBLE));

This value '1000' (along with the hopefully-soon-to-be-retired '500')
permeate your code. We disaffectionately refer to this as a "magic number"
that would be better off represented as a symbolic constant. In C, if it is
used as an array dimension, you're pretty much constrained to using
#define'd symbols, but that's better than littering the code with '1000's.

Another severe flaw in your organization is the fact you're iterating
across the entire 1000-element range of your verse array regardless of how
much data has actually been loaded (which you don't seem to actually find
out from any of the code here). What you want to do in cases like this is
check how much you've loaded (if any), remember that value, and use it as
the limit for those loops which iterate over the data. Using something like
the version of fillverse() I'll show you below, you'd manage that sort of
thing in the portion of your code that actually calls fillverse.

if (pverse == NULL)
{
printf ("It didn't work.\n");
return -1;

EXIT_FAILURE is the kosher thing to return from main() upon failure.

}

printf ("pverse = %d\n", (sizeof (BIBLE) * 1000));

If the malloc didn't fail, then it returned a pointer to the desired amount
of memory. In any case, what you're doing above is perhaps not what you
think you're doing; your text says "pverse = ", but you're /not/ displaying
the value of pverse. You're displaying the amount of memory you've obtained
from malloc, which pverse is now pointing to. You're not really learning
anything interesting here, and wouldn't be even if you /were/ displaying
the value of pverse. BTW, as I've recently learned via this group, the
right way to display the value of pverse (if you cared) would be:
printf("pverse = %p\n", (void *) pverse);

for (x = 0; x < 1000; x++)
fillverse (pverse);

Whoa. You're calling fillverse 1000 times; within fillverse, you've got a
1000-iteration loop going with an inner 500 iteration loop. What's wrong
with this picture?

Now it is time to re-think the data structure and your approach to using
it. Start by having an abstract idea of what "fillverse" is supposed to do.
Is it to "fill a verse", or "fill all verses"? If the former (I think
that's the better choice here), then think in terms of passing a pointer to
just where the /one/ verse is to go. That may look something like this:
for (x = 0; x < whatever; x++)
fillverse(&pverse[x]); /* pass pointer to THE verse to fill */

Within fillverse, /if/ you decide to make the text data member a
pointer-to-char and perform dynamic allocation (and I suggest you do), then
there will be a malloc performed in each call to fillverse.

for (x = 0; x < 1000; x++)
printverse (pverse);

Same idea (now, you didn't show us printverse, so I can't check its
signature for consistency, but the overall approach will shadow whatever
you do with fillverse).

free (pverse);

Again, if you go with the dynamic text, that I suggest you use a function
for this, and you'll have to free each of the text pointers (one within
each element of the dynamic pverse array) before you free the pverse
pointer as the last step.

return 0;
}

void fillverse (BIBLE *pverse)
{
int x, y;

for (x = 0; x < 1000; x++)
for (y = 0; y < 500; y++)
{
printf ("%s\n", pverse.text);
pverse.text[y] = '\0';
}

This is severely problematic. You shouldn't ever really be needing to
iterate over every single character in a string the way your inner loop
seems to be doing; almost always there will be some library function (or
some function of your own devising) that you'd employ to process a "line",
or a "verse", or whatever (whether it be input or output, and this function
seems to be a bit confused as to which it is doing). If this function is
read in "a verse", in fact there ought not be any loops at /all/, but just
something like this:

#include <string.h>
#define MAX_VERSE 500 /* longest verse you expect + 1 */
FILE *fp; /* assume you've got this managed somewhere... */

BIBLE *fillverse(BIBLE *pverse)
{
char buffer[MAX_VERSE];
if (fgets(buffer, MAX_VERSE, fp) == NULL)
{
printf("EOF reached.\n");
return NULL;
}
else
if ((pverse->text = malloc(strlen(buffer) + 1)) == NULL)
{
printf("Allocation error. Giving up.\n");
exit(EXIT_FAILURE);
}
return pverse;
}

The way I've set it up, the function returns NULL to indicate it has loaded
all available text from the input stream (fp).

}

*----------------------------------------*

I know that in the function fillverse, that I have the pverse.text[y]
wrong. I've been playing with this for several hours trying to find the
right way to do what I'm wanting to do with this.

Please give my any advice on style or whatever you think may be
appropriate.

I'm going to stop here and let you digest this. Check back as the project
evolves!

Good luck,
-leor

 

[Jens.Toerring]

I'm working on a quick program to bring the Bible into memory from a
text file. Anyway, I have three questions to ask.

First, is my implementation of malloc () correct in the program to
follow?

I can't see any "implementation" of malloc(), I guess you mean
"invocation";-)

Second, have I correctly passed the structure's pointer to the
functions in this program?

Last, how do I go about initializing the structure variables in the
given function?

Here's the program:

#include <stdio.h>
#include <stdlib.h>

struct bibtxt
{
char book[32];
int chapter;
int verse;
char text[500];
};

One remark: It's usually not good style to use such "magic" numbers like
32 and 500. If you need them it tends to be preferable to #define a sym-
bolic name for then and then use that names in your program - that will
make it a lot easier to read and also to modify (just think what happens
if you find a verse with more than 499 characters and then having to
hunt through your program to replace the 500's by a new value). So I
would recommend to use e.g.

#define MAX_BOOK_NAME_LENGTH 32
#define MAX_VERSE_LENGTH 500
#define MAX_VERSE_COUNT 1000

(In a later stage of the development of your program you probably will
find that it's going to be more practical to make both that 'book' and
'text' members of the structure char pointers and only allocate as much
memory for them as you really need instead of using some fixed-length
arrays.)

typedef struct bibtxt BIBLE;

void fillverse (BIBLE *pverse);

int main (void)
{
BIBLE *pverse;
int x;

pverse = malloc (1000 * sizeof (BIBLE));
if (pverse == NULL)
{
printf ("It didn't work.\n");
return -1;

Better make that

return EXIT_FAILURE;

so you can be sure that a value meaning falure will be returned on
whatever operating system you are going to compile the program.

}

The way you call malloc() as well as checking the return value (and
including the necessary header files) is fine. The only thing I would
do differently is to use

pverse = malloc( 1000 * sizeof *pverse );

since if written like this you don't have to worry about replacing
'BIBLE' by something else in case you decide later to make 'pverse'
a pointer to an object of a different type.

printf ("pverse = %d\n", (sizeof (BIBLE) * 1000));

I guess you know that what you're printing here isn't the location to
which 'pverse' is pointing to but the number of allocated bytes (or,
to be precise, the number of chars). Moreover, the value you get from
sizeof() is of type 'size_t' which isn't an int. It is always unsigned
and could also be e.g. a long int. So it might be prudent to cast the
second argument of printf() to the largest unsigned integral type
available on your machine and use the appropriate conversion specifier,
perhaps like this:

printf( "pverse = %lu\n", ( unsigned long ) ( sizeof *pverse * 1000 ) );

for (x = 0; x < 1000; x++)
fillverse (pverse);

If I guess correctly you don't want to "fill" the first structure over
and over again but all of them one after another. In that case (and if
fillverse() is actually meant to work on a single structure at a time)
what you need here is either

for ( x = 0; x < 1000; x++ )
fillverse( pverse + x );

or

for ( x = 0; x < 1000; x++ )
fillverse( &pverse[ x ] );

("pverse + x" and "&pverse[ x ]" are equivalent, both point to the
x-th structure in the array of structures you allocated).

for (x = 0; x < 1000; x++)
printverse (pverse);

There's probably the same problem here...

free (pverse);

return 0;

To stay on the safe side you should use

return EXIT_SUCCESS;

here, a return value of 0 does not necessarily mean success under all
operating systems while EXIT_SUCCESS will be correct wherever you
compile the program.

}

void fillverse (BIBLE *pverse)
{
int x, y;

for (x = 0; x < 1000; x++)

Why do you loop over 'x' when it's never used in the following? You
are just heating up your CPU;-) That way you just repeat the inner
loop a thousand times on the same structure.

for (y = 0; y < 500; y++)
{
printf ("%s\n", pverse.text);
pverse.text[y] = '\0';
}
}

That doesn't make much sense. First of all, going by the name of the
function I would tend to assume that it's meant to put some data in
the structure. And since it's called directly after allocating memory
the 'text' member of your 'bibtxt' structure 'pverse' is pointing to
contains just random data and nothing you could print and, wprse, there
is also nothing that guarantees that there's a '\0' in the 'text' array,
so in the first invocation of printf() you could even end up with
accessing memory beyond the end of that array unless there's a '\0'
accidentally already in the memory you got from malloc(). The next
problem is that 'pverse' is a pointer so you would need

printf( "%s\n", pverse->text );

and, in the following line,

pverse->text[ y ] = '\0';

Since you then would put a '\0' into the first element later calls of
prinf() are ok (but they are rather useless, there's not much to see
when you print just the empty string).

All what that loop (after the replacing the dot by the arrow operator)
does is to initialize all elements of the 'text' array with '\0'. I do
not see what this is good for but if it's really what you want to do
using memset() would be probably a lot more efficient. And if you would
use calloc() instead of malloc() you would get the same effect without
any additional code at all.

If the purpose of the fillverse() function is actually to zero out the
'text' member arrays of all the 'bibtxt' structures you allocated then
you could write it as

void fillverse( BIBLE *pverse )
{
int i;

for ( i = 0; i < 1000; i++ )
memset( pverse[ i ].text, 0, 500 );
}

or, if you prefer to loop over the elements of 'text'

void fillverse( BIBLE *pverse )
{
int i, j;

for ( i = 0; i < 1000; i++ )
for ( j = 0; j < 500; j++ )
pverse[ i ].text[ j ] = '\0';
}

Please note that now you have to use the dot instead of the arrow
operator. While the unadorned 'pverse' is a pointer, 'pverse[ i ]'
is the i-th structure of the array of structures 'pverse' is poin-
ting to.

If you write fillverse() like this you only have to call it once,
passing it the address of the memory you allocated, and not a
thousand times!

If you instead want to loop in main() you would rewrite fillverse()
as

void fillverse( BIBLE *pverse )
{
memset( pverse->text, 0, 500 );
}

and call it from main() like this

for ( i = 0; i < 1000; i++ )
fillverse( pverse + i );

Please give my any advice on style or whatever you think may be
appropriate.

Since it's a bit unclear what exactly the fillverse() function is
supposed to do (I don't think that you really want to zero out
the 'text' member of the first of your allocated structures a
million times, don't you?) it's hard to tell what to do instead.
Can you expain a bit more detailed? Since I assume that you want
to initialize the members of the structures in that function you
also will have to tell where you plan to get that data from etc.

Regards, Jens

 

[Mannequin*]

printf ("pverse = %d\n", (sizeof (BIBLE) * 1000));

This was just to see that my math was right. I wasn't aiming to display
the contents of pverse, just the size that it got malloc'ed to.

for (x = 0; x < 1000; x++)
fillverse (pverse);

for (x = 0; x < 1000; x++)
printverse (pverse);

Just for future replies, I did fix this. I was quickly throwing up this
code so I could post the relevant parts here and mistakenly put the for
() loops in.

One other thing is that the function fillverse will eventually fill the
structure with content. I was just using it in the code I gave you to
test how to go about doing that. I figured that the NULL character would
be a nice way to do that.

Thanks for pointing that out. I also want to send a big thank you to
Jens and Leor for helping me through some of the structure problems I
was having.

-M.

 

[Ben Pfaff]

struct bibtxt
{
char book[32];
int chapter;
int verse;
char text[500];
};

One remark: It's usually not good style to use such "magic" numbers like
32 and 500. If you need them it tends to be preferable to #define a sym-
bolic name for then and then use that names in your program - that will
make it a lot easier to read and also to modify (just think what happens
if you find a verse with more than 499 characters and then having to
hunt through your program to replace the 500's by a new value).
[...]

For sizes of objects, sometimes I disagree. It is often just as
easy and just as clear to write
char buffer[32];
and then later write `sizeof buffer' instead of first writing
#define BUFFER_SIZE 32
char buffer[BUFFER_SIZE];
and then later writing `BUFFER_SIZE'. It also avoids defining
another global name (the macro name).

Structure members are a little different, though, because there
is no straightforward way, as far as I know, to find the size of
a structure member unless you have an object of that type handy.

 

[Jeremy Yallop]

Structure members are a little different, though, because there
is no straightforward way, as far as I know, to find the size of
a structure member unless you have an object of that type handy.

sizeof ((struct foo *)0)->member

Jeremy.

 

[Ben Pfaff]

sizeof ((struct foo *)0)->member

I said "straightforward", not "awful".

 

[Barry Schwarz]

Hi all,
I'm working on a quick program to bring the Bible into memory from a
text file. Anyway, I have three questions to ask.

First, is my implementation of malloc () correct in the program to
follow?

Second, have I correctly passed the structure's pointer to the
functions in this program?

Last, how do I go about initializing the structure variables in the
given function?

You need to open the text file (look up fopen). You need to read data
from the file. You need to determine whether that data represents a
new book, a new chapter, a new verse number, or the text for the
current verse (depending on how your text file is structured, you may
want to use fgets, fscanf, fgetc, or possibly even fread).

If the data is a new book, you could save the book name in a local
string and set chapter and verse to 1.

If the data is a new chapter, you could save the chapter number in
a local int (or just increment it) and set the verse to 1.

If the data is a new verse number, you could save that in a local
int (or just increment it).

If the data is the text for a verse, you could copy it to the next
available element of your allocated array along with the book,
chapter, and verse number, each in the appropriate member of the
struct.

When all done, you should close the file (fclose).

Here's the program:

*----------------------------------------*

#include <stdio.h>
#include <stdlib.h>

struct bibtxt
{
char book[32];
int chapter;
int verse;
char text[500];
};

typedef struct bibtxt BIBLE;

void fillverse (BIBLE *pverse);

int main (void)
{
BIBLE *pverse;
int x;

pverse = malloc (1000 * sizeof (BIBLE));
if (pverse == NULL)
{
printf ("It didn't work.\n");
return -1;
}

printf ("pverse = %d\n", (sizeof (BIBLE) * 1000));

for (x = 0; x < 1000; x++)
fillverse (pverse);

Since pverse points to all the struct, why are you calling fillverse
1000 times with the exact same argument?

for (x = 0; x < 1000; x++)
printverse (pverse);

Function not defined. If it is of a design similar to fillverse, you
will not need to call it 1000 times.

free (pverse);

return 0;
}

void fillverse (BIBLE *pverse)
{
int x, y;

for (x = 0; x < 1000; x++)
for (y = 0; y < 500; y++)
{

Notice that x is never used in the loop. Frequently an indication of
something wrong.

printf ("%s\n", pverse.text);

None of the text members of the allocated arrays has been initialized.
After you correct the syntax error described below, this would still
invoke undefined behavior. Even if you initialized the text members,
why would you want to print each one 500 times?

The . operator requires a struct operand on the left. pverse is not a
struct, it is a pointer to struct. Since you are in the middle of a
loop through each element of the allocated array, it seems likely that
you want "pverse[x].text". Alternately but not recommended is
"(pverse+x)->text". Definitely recommended against is
"*(pverse+x).text".

pverse.text[y] = '\0';

It is normally sufficient to initialize only the first char in a
string to '\0'. You could eliminate the y loop and simply say
pverse[x].text[0] = '\0';

However, if the code that stores the actual text in the array does not
use string functions that automatically include the terminal '\0', you
will need to provide it yourself. But in this case, it usually better
to add a single '/0' at the end of the string in that code rather than
initialize every element of the array here.

}
}

*----------------------------------------*

I know that in the function fillverse, that I have the pverse.text[y]
wrong. I've been playing with this for several hours trying to find the
right way to do what I'm wanting to do with this.

Please give my any advice on style or whatever you think may be
appropriate.

Thanks!
-M.

<<Remove the del for email>>

 

[Mannequin*]

Again, if you go with the dynamic text, that I suggest you use a
function for this, and you'll have to free each of the text pointers
(one within each element of the dynamic pverse array) before you free
the pverse pointer as the last step.

Would a function like this be the correct way to free the memory for
each of the text pointers? (Assuming that the structure (pverse) was set
up with char *text instead of char text[500].)

/*--------------------------------------------------------------------*/
void txtfree (BIBLE *pverse, int nov) /* int nov = number of */
{ /* verses in the text */
int x;

for (x = 0; x < nov; x++)
free (pverse[x].text); /* Free the verse memory */

free (pverse); /* Free the structure memory */
}
/*--------------------------------------------------------------------*/

Thanks again.
-M.

 

[Jeremy Yallop]

I said "straightforward", not "awful".

It looks pretty perspicuous from over here. How long did it take you
to understand it?

Jeremy.

 

[Jens.Toerring]

Would a function like this be the correct way to free the memory for
each of the text pointers? (Assuming that the structure (pverse) was set
up with char *text instead of char text[500].)

/*--------------------------------------------------------------------*/
void txtfree (BIBLE *pverse, int nov) /* int nov = number of */
{ /* verses in the text */
int x;

for (x = 0; x < nov; x++)
free (pverse[x].text); /* Free the verse memory */

free (pverse); /* Free the structure memory */
}
/*--------------------------------------------------------------------*/

That function is completely fine (assuming that you really allocated
all the memory you are free()ing, i.e. you allocated memory for each
of them). The only thing I would object to is the name of the function,
when you call it txtfree() I would expect it to just free the memory
allocated for the 'text' members and not also the pointer to the array
of structures. But, of course, picking function names is up to you,
just don't forget to also free memory you may have allocated for the
'book' members (in case you also made 'book' a char pointers instead of
keeping it a fixed-length array) because after you've free()ed 'pverse'
you won't be able get at them anymore.

Regards, Jens

 

[Rob Thorpe]

sizeof ((struct foo *)0)->member

Jeremy.

Nice trick, I didn't know that.