Newbie: Array of pointers to strings questions.

[Longfellow]

Newbie here...

Trying to wrap my head around what's going on here. Read through the
FAQ until thoroughly confused. Got some insight from K&R2, no help from
D&D, H&S, CU or CT&P, at least that I could understand. So have done
due diligence, I think.

What I'm trying to do is extract information from one file and insert
that information into other files. The first file has a string of
records, each of which have an ID and for each, there exists another
file. For each record, information is extracted and inserted into the
relevant file. There can be any number of pieces of information for
each record, and the information in each case is extracted as a string.

So I figured that I need an array of pointers to each string that can be
created dynamically for each record as it is read. For the insertion,
the relevant file is read into a temp file, where the extracted strings
are inserted appropriately, and then the relevant file is removed and
replaced (rename()) by the temp file. This is completed for each record
before moving on to the next.

Works like a charm, but handling the array was a problem.

The following code is an example of my undoubtedly very ugly solution.
I can't figure out why it works, nor can I comprehend what the code
should be. I was paying attention to freeing allocated memory in a
timely manner, which affected the solution somehow.

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

const char *line1 = "This is line one.";
const char *line2 = "This is line two.";
const char *line3 = "This is line three.";
const char *line4 = "This is line four.";

char *array[4]

char *ptr;

printf("\nThis is the individual print-outs:\n");

ptr = malloc(sizeof line1);
strcpy(ptr, line1);
array[0] = malloc(sizeof line1);
array[0] = ptr;
free(ptr);

array[0] = malloc(sizeof line1);
strcpy(array[0], line1);
printf("array[0] is: %s\n", array[0]);

ptr = malloc(sizeof line2);
strcpy(ptr, line2);
array[1] = malloc(sizeof line2);
array[1] = ptr;
free(ptr);

array[1] = malloc(sizeof line2);
strcpy(array[1], line2);
printf("array[1] is: %s\n", array[1]);

ptr = malloc(sizeof line3);
strcpy(ptr, line3);
array[2] = malloc(sizeof line3);
array[2] = ptr;
free(ptr);

array[2] == malloc(sizeof line3);
strcpy(array[2], line3);
printf("array[2] is: %s\n", array[2]);

ptr = malloc(sizeof line4);
strcpy(ptr, line4);
array[3] = malloc(sizeof line4);
array[3] = ptr;
free(ptr);

array[3] == malloc(sizeof line4);
strcpy(array[3], line4);
printf("array[3] is: %s\n", array[3]);

printf("\nThis is the main body print loop:\n");
for (i=0; i<4; ++i)
printf("array[%d] is %s\n", i, array[i]);

printit();

for (i=0; i<4; ++i)
free(array[i]);

free(ptr);

return 0;
}

void printit() {

int i;

printf("\nThis is the 'printit()' function print loop:\n");

for(i=0; i<4; ++i)
printf("array[%d] is: %s\n", i, array[i]);
}

Running this produces:

This is the individual print-outs:
array[0] is: This is line one.
array[1] is: This is line two.
array[2] is: This is line three.
array[3] is: This is line four.

This is the main body print loop:
array[0] is This is line one.
array[1] is This is line two.
array[2] is This is line three.
array[3] is This is line four.

This is the 'printit()' function print loop:
array[0] is: This is line one.
array[1] is: This is line two.
array[2] is: This is line three.
array[3] is: This is line four.

Delete or change any part of the array handling sections and something
doesn't work correctly.

What should the code be to produce the above results?

Thanks all,

Longfellow

[resemb...@gmail.com]

ptr will have the size of a memory address location in this case since
line1 is just a constant pointer to a character. Thus, on most 32-bit
machines ptr will have a size of 4 bytes, not sufficient enough to hold
the size of line1. What you could do is use strlen of line1 to
determine the size of what to allocate.

[Christian Kandeler]

Longfellow wrote:

> const char *line1 = "This is line one.";
> const char *line2 = "This is line two.";
> const char *line3 = "This is line three.";
> const char *line4 = "This is line four.";
>
> char *array[4]
>
> char *ptr;
>
> printf("\nThis is the individual print-outs:\n");

I assume main() starts somewhere before this line.

> ptr = malloc(sizeof line1);

This will allocate sizeof(char *) bytes, which I'm pretty sure is not what
you want. Try this;

ptr = malloc(strlen(line1) + 1);

Alternatively, you can make line1 an array.

> strcpy(ptr, line1);
> array[0] = malloc(sizeof line1);
> array[0] = ptr;
> free(ptr);

Huh? You are allocating memory for array[0] and then you overwrite it with
ptr, throwing it away forever. This is called a memory leak. Simply lose
the malloc() here, it is completely unnecessary. Or better yet, leave this
malloc(), remove the ptr variable (and its malloc()) altogether, and do the
strcpy() directly from line1 to array[0]. (Also note that the free()
operation renders the assignment in the line above it senseless.)

> array[0] = malloc(sizeof line1);
> strcpy(array[0], line1);

Oh, _now_ you do the right thing (minus the sizeof mistake). But why all the
superfluous malloc()s before?

> [ the same for the other array elements ]

All in all, it seems to me that you have not really understood pointers
and/or dynamic allocation of memory.

Christian

[Longfellow]

On 2005-05-11, Christian Kandeler <christian...@hob.de_invalid> wrote:
<snip>

Sorry about the example, which was flawed in more ways than I saw. It's
rather obvious that I don't really understand this stuff, but I'm
learning ;)

Here's the corrected version with the changes you specify:

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

const char *line1 = "This is line one.";

const char *line2 = "This is line two.";
const char *line3 = "This is line three.";
const char *line4 = "This is line four.";

char *array[4];

void printit();

int main(void)
{
int i;

printf("\nThis is the individual print-outs:\n");

array[0] = malloc(sizeof strlen(line1)+1);

strcpy(array[0], line1);
printf("array[0] is: %s\n", array[0]);

array[1] = malloc(sizeof strlen(line2)+1);

strcpy(array[1], line2);
printf("array[1] is: %s\n", array[1]);

array[2] = malloc(sizeof strlen(line3)+1);

strcpy(array[2], line3);
printf("array[2] is: %s\n", array[2]);

array[3] = malloc(sizeof strlen(line4)+1);

strcpy(array[3], line4);
printf("array[3] is: %s\n", array[3]);

printf("\nThis is the main body print loop:\n");
for (i=0; i<4; ++i)
printf("array[%d] is %s\n", i, array[i]);

printit();

for (i=0; i<4; ++i)
free(array[i]);

return 0;
}

void printit() {

int i;

printf("\nThis is the 'printit()' function print loop:\n");

for(i=0; i<4; ++i)
printf("array[%d] is: %s\n", i, array[i]);
}

This is the result:

This is the individual print-outs:
array[0] is: This is line one.
array[1] is: This is line two.
array[2] is: This is line three.
array[3] is: This is line four.

This is the main body print loop:
array[0] is This is line
array[1] is This is line
array[2] is This is line

array[3] is This is line four.

This is the 'printit()' function print loop:
array[0] is: This is line

array[1] is: This is line

array[2] is: This is line

array[3] is: This is line four.

What am I missing?

Thanks,

Longfellow

[Kevin Bagust]

Longfellow wrote:
> On 2005-05-11, Christian Kandeler <christian...@hob.de_invalid> wrote:
> <snip>
>
> Sorry about the example, which was flawed in more ways than I saw. It's
> rather obvious that I don't really understand this stuff, but I'm
> learning ;)
>
> Here's the corrected version with the changes you specify:

<snip>

>
> array[0] = malloc(sizeof strlen(line1)+1);
> strcpy(array[0], line1);
> printf("array[0] is: %s\n", array[0]);

<snip>

>
> What am I missing?

Two things.

1) You do not want the sizeof in the malloc call. With the sizeof there
you are getting the wrong number of bytes.

2) You need to check the return value of malloc for NULL, before you use it.

Which gives you the following code to replace the quoted code:

array[0] = malloc( strlen( line1 ) + 1 );
if ( NULL == array[0]) {
printf( "Unable to get memory\n" );
exit( 0 );

}
strcpy(array[0], line1);
printf("array[0] is: %s\n", array[0]);

Kevin.

[Targeur fou]

Longfellow a écrit :

> On 2005-05-11, Christian Kandeler <christian...@hob.de_invalid>
wrote:
> <snip>
>
> Sorry about the example, which was flawed in more ways than I saw.
It's
> rather obvious that I don't really understand this stuff, but I'm
> learning ;)
>
> Here's the corrected version with the changes you specify:
>
> #include <stdio.h>
> #include <stdlib.h>
> #include <string.h>
>
> const char *line1 = "This is line one.";
> const char *line2 = "This is line two.";
> const char *line3 = "This is line three.";
> const char *line4 = "This is line four.";
>
> char *array[4];

Initializing your array of pointers is a good pratice:
char * array[4] = { NULL, NULL, NULL, NULL };

>
> void printit();
>
> int main(void)
> {
> int i;
>
> printf("\nThis is the individual print-outs:\n");
>
> array[0] = malloc(sizeof strlen(line1)+1);

1. A good practice (highly recommended...mandatory in fact):
You shall test the return of malloc.

2. The unary sizeof and + operators have the same precedence.
strlen returns a size_t (an unsigned integer type). Therefore, you're
adding the size of a size_t (because you're applying sizeof on the
strlen return) and one (For instance, sizeof(size_t) = 4 with my
implementation, and that would lead to allocate only 5 bytes). You
don't need the sizeof here (and remember that sizeof(char)=1, always).

A correct way to do it:
array[0] = malloc(strlen(line1)+1);
if (array[0] != NULL) {
strcpy(array[0], line1);
}

> printf("array[0] is: %s\n", array[0]);

At last, you should print array[X] if it's not null only.
The same thing applies for your other array elements.
printf("array[0] is: %s\n,(array[0]!= NULL)? array[0] : "NULL");

HTH
Regis

[pete]

/* BEGIN new.c */

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

#define NMEMB(array) (sizeof array / sizeof *array)

void printit(char **s, size_t n);
void free_ptrs(char **s, size_t nmemb);

int main(void)
{
char *line[] = {
"This is line one.","This is line two.",
"This is line three.","This is line four."
};
char *array[NMEMB(line)];
unsigned i, j;

puts("\nThis is the individual print-outs:");
for (i = 0; i != NMEMB(line); ++i) {
array[i] = malloc(strlen(line[i]) + 1);
if (array[i] == NULL) {
break;
} else {
strcpy(array[i], line[i]);
printf("array[%u] is: %s\n", i, array[i]);
}
}
puts("\nThis is the main body print loop:");
for (j = 0; j != i; ++j) {
printf("array[%u] is %s\n", j, array[j]);
}
printit(array, j);
free_ptrs(array, i);
return 0;
}

void printit(char **s, size_t j)
{
unsigned i;

puts("\nThis is the 'printit()' function print loop:");
for (i = 0; i != j; ++i) {
printf("array[%u] is: %s\n", i, s[i]);
}
}

void free_ptrs(char **s, size_t nmemb)
{
while (nmemb-- != 0) {
free(s[nmemb]);
}
}

/* END new.c */

--
pete

[Longfellow]

On 2005-05-11, Targeur fou <rtro...@yahoo.fr> wrote:
<snip>

Thanks to all, especially Kevin Bagust and Targeur fou!

Turns out that I understand more than I thought, as your explanations
made perfect sense. That will make it possible for me to eventually
know this stuff well enough to use it correctly, or so I hope. The
error handling code is much appreciated: I do include what of it that I
understand well enough to use.

I've no idea why the original code worked, however, but perhaps I can
puzzle that out later. For now, the corrections all work as they
should.

Again, thanks to all you guys for the critique and instruction!

Longfellow

[CBFalconer]

Kevin Bagust wrote:

> Longfellow wrote:
> > Christian Kandeler <christian...@hob.de_invalid> wrote:
>
> <snip>
>

>> array[0] = malloc(sizeof strlen(line1)+1);
>> strcpy(array[0], line1);
>> printf("array[0] is: %s\n", array[0]);
>
> <snip>
>

> 1) You do not want the sizeof in the malloc call. With the
> sizeof there you are getting the wrong number of bytes.

At least with me it segfaulted. I stared and stared at that code
without seeing the silly error. I wonder how many readers had the
same optical illusion.

--
"If you want to post a followup via groups.google.com, don't use
the broken "Reply" link at the bottom of the article. Click on
"show options" at the top of the article, then click on the
"Reply" at the bottom of the article headers." - Keith Thompson

[Longfellow]

On 2005-05-11, pete <pfi...@mindspring.com> wrote:
> Longfellow wrote:

<snip>

Okay, this takes the whole thing to another level, or so it seems to me.
I've saved this for further study. Thanks!

Longfellow

[Christian Kandeler]

CBFalconer wrote:

>> 1) You do not want the sizeof in the malloc call. With the
>> sizeof there you are getting the wrong number of bytes.
>
> At least with me it segfaulted. I stared and stared at that code
> without seeing the silly error. I wonder how many readers had the
> same optical illusion.

At least one ;)

Christian

[Targeur fou]

Christian Kandeler a écrit :

At least four :-D (You, Kevin, Chuck and me...and probably other guys).

But the fact is that I can not give a good explanation for this issue.

--
Targeur
"Assistance Targeur à 55 m, allume les franges latérales !. Targeur,
fana de plongée et de C."

[Targeur fou]

Christian Kandeler a écrit :

(Sorry Pete), at least five !!

[pete]

You're welcome. I tried to make it worthwhile.
There are no external objects (eg. the global arrays)
in this version.
And no magic numbers (constants other than 1 or 0).

--
pete

[CBFalconer]

I like to think it is an error I would never make in writing it.
Yet it is surprisingly elusive when reading other peoples code.

[CBFalconer]

pete wrote:
>
... snip ...

>
> void free_ptrs(char **s, size_t nmemb)
> {
> while (nmemb-- != 0) {
> free(s[nmemb]);
> }
> }

A slight reorganization adds a world of safety, and makes it do the
right thing when nmemb is zero.

void free_ptrs(char **s, size_t nmemb)
{

while (nmemb) free(s[--nmemb]);
}

--
Some informative links:
news:news.announce.newusers
http://www.geocities.com/nnqweb/
http://www.catb.org/~esr/faqs/smart-questions.html
http://www.caliburn.nl/topposting.html
http://www.netmeister.org/news/learn2quote.html

[Christian Kandeler]

CBFalconer wrote:

> I like to think it is an error I would never make in writing it.
> Yet it is surprisingly elusive when reading other peoples code.

It is elusive _because_ you would never write it that way. The OP is
possibly the first person ever to have used the sizeof operator on a
function call.

Christian

[Alan Balmer]

On Wed, 11 May 2005 17:17:42 GMT, CBFalconer <cbfal...@yahoo.com>
wrote:

>Kevin Bagust wrote:
>> Longfellow wrote:
>> > Christian Kandeler <christian...@hob.de_invalid> wrote:
>>
>> <snip>
>>
>>> array[0] = malloc(sizeof strlen(line1)+1);
>>> strcpy(array[0], line1);
>>> printf("array[0] is: %s\n", array[0]);
>>
>> <snip>
>>
>> 1) You do not want the sizeof in the malloc call. With the
>> sizeof there you are getting the wrong number of bytes.
>
>At least with me it segfaulted. I stared and stared at that code
>without seeing the silly error. I wonder how many readers had the
>same optical illusion.

I spotted it, but only because I've just finished fixing up a program
where the original author consistently used sizeof when he should have
used strlen. Amazingly, it would sometimes run for a while without
crashing. But only sometimes ;-)

--
Al Balmer
Balmer Consulting
removebalmerc...@att.net

[Lawrence Kirby]

On Thu, 12 May 2005 13:54:23 +0000, CBFalconer wrote:

> pete wrote:
>>
> ... snip ...
>>
>> void free_ptrs(char **s, size_t nmemb)
>> {
>> while (nmemb-- != 0) {
>> free(s[nmemb]);
>> }
>> }
>
> A slight reorganization adds a world of safety, and makes it do the
> right thing when nmemb is zero.
>
> void free_ptrs(char **s, size_t nmemb)
> {
> while (nmemb) free(s[--nmemb]);
> }

The only difference I see between the 2 versions is the value of nmemb
after the loop terminates. But since that value isn't used and is well
defined in both cases they are functionally equivalent.

I do think your version is cleaner however.

Lawrence

[Longfellow]

ROFL!!!

Oh, almost certainly not! The fact is that I'm probably the first
person to have done so, and then have the hubris to post here before
awakening to my blunder! Actually, make that "post here *in order to
awaken* to my blunder". :D

Update: Ran the corrected code last night on a file containing several
thousand records. Memory use never budged (array freed at the end of
each record), and the thing fairly flew along. First time I've written
a program to handle that much data, and it was an exhilarating
experience to watch it run streams of screen output (one line per record
and one line for each insertion) so fast it outran the screen update.

Result: More hubris!! I'm now contemplating undertaking a real
application!! That should keep me busy for the next few.... lol!!

Again, thanks to all for their responses.

Longfellow

[Keith Thompson]

I doubt it. In fact, I can imagine applying the sizeof operator to a
function call actually being useful. It gives you the size in bytes
of the function's result without calling the function (since the
operand of sizeof isn't evaluated unless it's a VLA).

--
Keith Thompson (The_Other_Keith) ks...@mib.org <http://www.ghoti.net/~kst>
San Diego Supercomputer Center <*> <http://users.sdsc.edu/~kst>
We must do something. This is something. Therefore, we must do this.

[CBFalconer]

Apologies. The original does work correctly. I was thinking
(faultily) that it would do the unsigned wrap around thing and run
indefinitely for an input value of zero, while trying to access
non-existing array members.

[Christian Kandeler]

Keith Thompson wrote:

> In fact, I can imagine applying the sizeof operator to a
> function call actually being useful. It gives you the size in bytes
> of the function's result without calling the function (since the
> operand of sizeof isn't evaluated unless it's a VLA).

While you are technically correct (I wasn't aware that the function doesn't
get called), I would find such as construct rather confusing, if not
obfuscating. IMHO, this outweighs the advantage of not having to hard code
the data type explicitly. I will therefore continue to write sizeof(int)
instead of sizeof printf("Hello World\n").

Christian