Queue question wih dyn mem alloc

[cerr]

Hi,
I wrote two functions to push() a character array onto an array and
to
pop() the first element[0] of that array again.
The array holding the elements should be dynamically extendible. So
my
this is my code, the push () function seems to work well but i have
questions for my pop() implementation.
int push(char **list, char *str, int curlen){
/*******************************************************
-Description-
This function allocates memory on an array for character
arrays to carry an array of strings. This function is
intended to be used with the pop function for a FIFO
(First In-First Out) data structure that stands for
the std::queue that is availabld in C++.
-Parameters-
char **list - char double pointer to the list carrying
the strings
char *str - char array pointing to the string that
should be added to the array list
int len - integer (global variable from the caller)
that carries the number of elements that
list already carries (required for offset
calculation)
-Return value-
integer - the int value returned by the function
represents the number of elements in list.
This will basically be curlen + 1.
*******************************************************/
char **temp;
temp = realloc(list,(curlen+1)*sizeof(*list));
if (temp==NULL){
printf("push(): Error reallocating memory for msglist\n");
for (i=Len;i>=0;i--) {
free(msglist);
Len--;
}
free(list);
return -1;
}
msglist=temp;
msglist[curlen]=malloc (strlen(str)+1);
strcpy(msglist[curlen],str);
return ++curlen;
}

//-------------------------------------------------------
int pop (char ** list, char *outstr, int curlen)
/*******************************************************
-Description-
This function pops the first value of an array
carrying character arrays. This function can be used
with the push function for a FIFO (First In-First Out)
data structure that stands for the std::queue
that is availabld in C++.
-Parameters-
char **list - char double pointer to the list carrying
the strings
char *outstr- char array pointing to a pre allocated
string where the first value of the array
will be copied to.
int len - integer (global variable from the caller)
that carries the number of elements that
list carries
-Return value-
integer - the int value returned by the function
represents the number of elements in list.
This will basically be curlen - 1.
*******************************************************/
{
int j=0;
int i=0;
char **temp;
if (curlen==0) {
printf("pop(): No element left in list\n");
outstr="";
return 0;
}
/* WARNING!!! Why does it seem to still work fine if there's fewer
bytes allocated to outstr than list[0] is long?*/
strcpy(outstr, list[0]);
for (j=1; j<curlen; j++){
temp = realloc(list,(curlen)*sizeof(*list));
if (temp==NULL){
printf("pop(): Error reallocating temp memory for msglist\n");
for (i=curlen;i>=0;i--) {
free(temp);
curlen--;
}
free(list);
return -1;
}
temp[j-1]=malloc (strlen(list[j])+1);
strcpy(temp[j-1],list[j]);
}
list=temp;
return --curlen;
}

Also, when i call the pop() function as below, what happens if "res"
isn't "long" enough?
char res[1024]={};
int Len;
Len=pop(msglist,res,Len);
Thanks for hints and suggestions!

 

[Eric Sosman]

Hi,
I wrote two functions to push() a character array onto an array and
to
pop() the first element[0] of that array again.
The array holding the elements should be dynamically extendible. So
my
this is my code,[...]

Are you sure? Even after adding #includes for the missing
headers, gcc says

gcc -Wall -W -std=c99 -pedantic -c foo.c
foo.c: In function 'push':
foo.c:31: error: 'i' undeclared (first use in this function)
foo.c:31: error: (Each undeclared identifier is reported only once
foo.c:31: error: for each function it appears in.)
foo.c:31: error: 'Len' undeclared (first use in this function)
foo.c:32: error: 'msglist' undeclared (first use in this function)

It seems what you've shown us is a sort of rough approximation
of your code. There are lots of things wrong with it (beyond the
obvious blunders gcc finds), but I don't feel like spending time
correcting some problem other than the one you're actually having.
If you'll try again, with *complete* and *actual* code, I may be
more able to help.

 

[cerr]

Hi,
I wrote two functions to push() a character array onto an array and
to
pop() the first element[0] of that array again.
The array holding the elements should be dynamically extendible. So
my
this is my code,[...]

     Are you sure?  Even after adding #includes for the missing
headers, gcc says

gcc -Wall -W -std=c99 -pedantic -c foo.c
foo.c: In function 'push':
foo.c:31: error: 'i' undeclared (first use in this function)
foo.c:31: error: (Each undeclared identifier is reported only once
foo.c:31: error: for each function it appears in.)
foo.c:31: error: 'Len' undeclared (first use in this function)
foo.c:32: error: 'msglist' undeclared (first use in this function)

     It seems what you've shown us is a sort of rough approximation
of your code.  There are lots of things wrong with it (beyond the
obvious blunders gcc finds), but I don't feel like spending time
correcting some problem other than the one you're actually having.
If you'll try again, with *complete* and *actual* code, I may be
more able to help.

Okay below i posted the whole source code from my little test app:
don't forget to compile it with the -pthread flag.
#include <stdio.h>
#include <stdlib.h>
#include <string.h>
#include <pthread.h>
int push(char**,char*, int);
int pop(char**,char*, int);
char **msglist;
int Len=0;
int i=0;
int done=0;
pthread_mutex_t log_mtx;
char* str="Novax";
char chrarr[100]={0};

//-------------------------------------------------------
int push(char **list, char *str, int curlen){
/*******************************************************
-Description-
This function allocates memory on an array for character
arrays to carry an array of strings. This function is
intended to be used with the pop function for a FIFO
(First In-First Out) data structure that stands for
the std::queue that is availabld in C++.
-Parameters-
char **list - char double pointer to the list carrying
the strings
char *str - char array pointing to the string that
should be added to the array list
int len - integer (global variable from the caller)
that carries the number of elements that
list already carries (required for offset
calculation)
-Return value-
integer - the int value returned by the function
represents the number of elements in list.
This will basically be curlen + 1.
*******************************************************/

char **temp;

temp = realloc(list,(curlen+1)*sizeof(*list));
if (temp==NULL){
printf("push(): Error reallocating memory for msglist\n");
for (i=Len;i>=0;i--) {
free(msglist);
Len--;
}
free(list);
return -1;
}
msglist=temp;
msglist[curlen]=malloc (strlen(str)+1);
strcpy(msglist[curlen],str);

return ++curlen;
}
//-------------------------------------------------------
int pop (char ** list, char *outstr, int curlen)
/*******************************************************
-Description-
This function pops the first value of an array
carrying character arrays. This function can be used
with the push function for a FIFO (First In-First Out)
data structure that stands for the std::queue
that is availabld in C++.
-Parameters-
char **list - char double pointer to the list carrying
the strings
char *outstr- char array pointing to a pre allocated
string where the first value of the array
will be copied to.
int len - integer (global variable from the caller)
that carries the number of elements that
list carries
-Return value-
integer - the int value returned by the function
represents the number of elements in list.
This will basically be curlen - 1.
*******************************************************/
{
int j=0;
int i=0;
char **temp;
if (curlen==0) {
printf("pop(): No element left in list\n");
outstr="";
return 0;
}
/* WARNING!!! Why does it seem to still work fine if there's fewer
bytes allocated to outstr than list[0] is long?*/
strcpy(outstr, list[0]);
for (j=1; j<curlen; j++){
temp = realloc(list,(curlen)*sizeof(*list));
if (temp==NULL){
printf("pop(): Error reallocating temp memory for msglist\n");
for (i=curlen;i>=0;i--) {
free(temp);
curlen--;
}
free(list);
return -1;
}
temp[j-1]=malloc (strlen(list[j])+1);
strcpy(temp[j-1],list[j]);
}
list=temp;

return --curlen;
}
//-------------------------------------------------------
void _run_logger(void){
int locLen=0;
char res[5]={0};
// char *res=NULL;
int s=0;
printf("THREAD started\n");
while (1) {
pthread_mutex_lock(&log_mtx);
locLen=Len;
pthread_mutex_unlock(&log_mtx);
if (locLen>0) {
/* WARNING! What happens if more bytes get set to 0 than there's
allocated space in res? */
memset(res,'\0',1024);
pthread_mutex_lock(&log_mtx);
Len=pop(msglist,res,Len);
printf("<<popped %s off from head of msglist, newLen%d\n",res,
Len);
pthread_mutex_unlock(&log_mtx);
}
else {
printf ("Queue is empty, locLen:%d\n",locLen);
for(s=0;s<1000000;s++);
}
}

}
//-------------------------------------------------------
int main (void){

pthread_mutex_init(&log_mtx, NULL);
int rc=0;
pthread_t threads;
int o=0;

rc = pthread_create(&threads, NULL, (void*)_run_logger,0);
if (rc){
printf("ERROR; return code from pthread_create() is %d\n", rc);
exit(-1);
}

for (i=0;i<=1000;i++){
sprintf(chrarr,"%s%dHello",str,i);
pthread_mutex_lock(&log_mtx);
Len=push(msglist, chrarr, Len);
printf(">>push %s newLen:%d\n",msglist[Len-1], Len);
pthread_mutex_unlock(&log_mtx);
for(o=0;o<100000;o++);
}
/* give the thread some time to process the leftover elements*/
for(o=0;o<10000000;o++);
return 0;
}

 

[Dann Corbit]

Hi,
I wrote two functions to push() a character array onto an array and
to
pop() the first element[0] of that array again.
The array holding the elements should be dynamically extendible. So
my
this is my code,[...]

     Are you sure?  Even after adding #includes for the missing
headers, gcc says

gcc -Wall -W -std=c99 -pedantic -c foo.c
foo.c: In function 'push':
foo.c:31: error: 'i' undeclared (first use in this function)
foo.c:31: error: (Each undeclared identifier is reported only once
foo.c:31: error: for each function it appears in.)
foo.c:31: error: 'Len' undeclared (first use in this function)
foo.c:32: error: 'msglist' undeclared (first use in this function)

     It seems what you've shown us is a sort of rough approximation
of your code.  There are lots of things wrong with it (beyond the
obvious blunders gcc finds), but I don't feel like spending time
correcting some problem other than the one you're actually having.
If you'll try again, with *complete* and *actual* code, I may be
more able to help.

Okay below i posted the whole source code from my little test app:
don't forget to compile it with the -pthread flag.
#include <stdio.h>
#include <stdlib.h>
#include <string.h>
#include <pthread.h>
int push(char**,char*, int);
int pop(char**,char*, int);
char **msglist;
int Len=0;
int i=0;
int done=0;
pthread_mutex_t log_mtx;
char* str="Novax";
char chrarr[100]={0};

//-------------------------------------------------------
int push(char **list, char *str, int curlen){
/*******************************************************
-Description-

--- Module: ptp.c (C)
_
char* str="Novax";
ptp.c(12) : Info 1776: Converting a string literal to char * is not
const safe
(initialization)
_
int push(char **list, char *str, int curlen) {
ptp.c(16) : Warning 578: Declaration of symbol 'str' hides symbol 'str'
(line
12)
ptp.c(12) : Info 830: Location cited in prior message
_
temp = realloc(list,(curlen+1)*sizeof(*list));
ptp.c(41) : Info 737: Loss of sign in promotion from int to unsigned int
_
free(list);
ptp.c(48) : Warning 449: Pointer variable 'list' previously deallocated
[Reference: file ptp.c: line 41]
ptp.c(41) : Info 831: Reference cited in prior message
_
}
ptp.c(56) : Note 952: Parameter 'str' (line 16) could be declared const
---
Eff. C++ 3rd Ed. item 3
ptp.c(16) : Info 830: Location cited in prior message
_
}
ptp.c(56) : Info 818: Pointer parameter 'str' (line 16) could be
declared as
pointing to const --- Eff. C++ 3rd Ed. item 3
ptp.c(16) : Info 830: Location cited in prior message
_
int i=0;
ptp.c(82) : Warning 578: Declaration of symbol 'i' hides symbol 'i'
(line 9)
ptp.c(9) : Info 830: Location cited in prior message
_
outstr="";
ptp.c(86) : Info 1776: Converting a string literal to char * is not
const safe
(assignment)
_
temp = realloc(list,(curlen)*sizeof(*list));
ptp.c(93) : Info 737: Loss of sign in promotion from int to unsigned int
_
free(temp);
ptp.c(97) : Warning 613: Possible use of null pointer 'temp' in left
argument
to operator '[' [Reference: file ptp.c: line 94]
ptp.c(94) : Info 831: Reference cited in prior message
_
free(list);
ptp.c(100) : Warning 449: Pointer variable 'list' previously deallocated
[Reference: file ptp.c: line 93]
ptp.c(93) : Info 831: Reference cited in prior message
_
list=temp;
ptp.c(106) : Info 771: Symbol 'temp' (line 83) conceivably not
initialized ---
Eff. C++ 3rd Ed. item 4
ptp.c(83) : Info 830: Location cited in prior message
_
while (1) {
ptp.c(117) : Info 716: while(1) ...
_
pthread_mutex_lock(&log_mtx);
ptp.c(118) : Warning 534: Ignoring return value of function
'pthread_mutex_lock(struct pthread_mutex_t_ **)' (compare with line
998,
file .\pthread.h)
..\pthread.h(998) : Info 830: Location cited in prior message
_
pthread_mutex_unlock(&log_mtx);
ptp.c(120) : Warning 534: Ignoring return value of function
'pthread_mutex_unlock(struct pthread_mutex_t_ **)' (compare with
line 1005,
file .\pthread.h)
..\pthread.h(1005) : Info 830: Location cited in prior message
_
memset(res,'\0',1024);
ptp.c(124) : Warning 419: Apparent data overrun for function 'memset
(void *,
int, unsigned int)', argument 3 (size=1024) exceeds argument 1
(size=5)
[Reference: file ptp.c: line 124]
ptp.c(124) : Info 831: Reference cited in prior message
_
pthread_mutex_lock(&log_mtx);
ptp.c(125) : Warning 534: Ignoring return value of function
'pthread_mutex_lock(struct pthread_mutex_t_ **)' (compare with line
998,
file .\pthread.h)
..\pthread.h(998) : Info 830: Location cited in prior message
_
pthread_mutex_unlock(&log_mtx);
ptp.c(129) : Warning 534: Ignoring return value of function
'pthread_mutex_unlock(struct pthread_mutex_t_ **)' (compare with
line 1005,
file .\pthread.h)
..\pthread.h(1005) : Info 830: Location cited in prior message
_
for (s=0;s<1000000;s++);
ptp.c(133) : Info 722: Suspicious use of ;
_
pthread_mutex_init(&log_mtx, NULL);
ptp.c(141) : Warning 534: Ignoring return value of function
'pthread_mutex_init(struct pthread_mutex_t_ **, struct
pthread_mutexattr_t_
*const *)' (compare with line 994, file .\pthread.h)
..\pthread.h(994) : Info 830: Location cited in prior message
_
rc = pthread_create(&threads, NULL, (void*)_run_logger,0);
ptp.c(146) : Warning 611: Suspicious cast
_
pthread_mutex_lock(&log_mtx);
ptp.c(154) : Warning 534: Ignoring return value of function
'pthread_mutex_lock(struct pthread_mutex_t_ **)' (compare with line
998,
file .\pthread.h)
..\pthread.h(998) : Info 830: Location cited in prior message
_
pthread_mutex_unlock(&log_mtx);
ptp.c(157) : Warning 534: Ignoring return value of function
'pthread_mutex_unlock(struct pthread_mutex_t_ **)' (compare with
line 1005,
file .\pthread.h)
..\pthread.h(1005) : Info 830: Location cited in prior message
_
for (o=0;o<100000;o++);
ptp.c(158) : Info 722: Suspicious use of ;
_
for (o=0;o<10000000;o++);
ptp.c(161) : Info 722: Suspicious use of ;

--- Global Wrap-up

Info 714: Symbol 'done' (line 10, file ptp.c) not referenced
ptp.c(10) : Info 830: Location cited in prior message

 

[Barry Schwarz]

Hi,
I wrote two functions to push() a character array onto an array and
to
pop() the first element[0] of that array again.
The array holding the elements should be dynamically extendible. So
my
this is my code, the push () function seems to work well but i have
questions for my pop() implementation.

snip an obviously incorrect copy of push

int pop (char ** list, char *outstr, int curlen)
/*******************************************************
-Description-
This function pops the first value of an array
carrying character arrays. This function can be used
with the push function for a FIFO (First In-First Out)
data structure that stands for the std::queue
that is availabld in C++.
-Parameters-
char **list - char double pointer to the list carrying
the strings
char *outstr- char array pointing to a pre allocated
string where the first value of the array
will be copied to.
int len - integer (global variable from the caller)

Apparently you meant curlen, not len. Furthermore, you have no idea
how the argument corresponding to this parameter is coded in the
calling function. The assumption that it is global seems particularly
unlikely since, if it were, there would be no need to pass it as an
argument.

that carries the number of elements that
list carries
-Return value-
integer - the int value returned by the function
represents the number of elements in list.
This will basically be curlen - 1.
*******************************************************/
{
int j=0;
int i=0;
char **temp;
if (curlen==0) {
printf("pop(): No element left in list\n");
outstr="";
return 0;
}
/* WARNING!!! Why does it seem to still work fine if there's fewer
bytes allocated to outstr than list[0] is long?*/

Bad karma. One of the worst manifestations of undefined behavior is
to appear to work as expected.

strcpy(outstr, list[0]);
for (j=1; j<curlen; j++){
temp = realloc(list,(curlen)*sizeof(*list));

How many times do you think you need to reallocate list? Hint: how
many items did you pop off the list?

Did you mean to use curlen-1 here? The only purpose is to reduce the
amount of dynamically allocated memory that list points to and this
allocates the exact same amount of memory.

if (temp==NULL){
printf("pop(): Error reallocating temp memory for msglist\n");
for (i=curlen;i>=0;i--) {
free(temp);

Since temp is NULL, it should be obvious that temp does not exist
and attempting to evaluate it invokes undefined behavior.

curlen--;

Why use i at all when curlen works just as well? As it stands, this
code is useless since curlen is not used after the for loop variable
has been initialized.

}
free(list);
return -1;
}
temp[j-1]=malloc (strlen(list[j])+1);
strcpy(temp[j-1],list[j]);

There is no need to move the strings themselves. It is sufficient to
copy the pointers in list to temp.

}
list=temp;

You have just created a massive set of memory leaks. All the dynamic
areas previously pointed to by the members of list are now
inaccessible.

return --curlen;

The extra code necessary to perform the side effect of the -- operator
is a waste since curlen will be destroyed as soon as the function
exits.

}

Also, when i call the pop() function as below, what happens if "res"
isn't "long" enough?

The same undefined behavior described above.

char res[1024]={};
int Len;
Len=pop(msglist,res,Len);
Thanks for hints and suggestions!

Cut and paste; don't retype.

 

[cerr]

Hi,
I wrote two functions to push() a character array onto an array and
to
pop() the first element[0] of that array again.
The array holding the elements should be dynamically extendible. So
my
this is my code, the push () function seems to work well but i have
questions for my pop() implementation.

snip an obviously incorrect copy of push

int pop (char ** list, char *outstr, int curlen)
/*******************************************************
  -Description-
  This function pops the first value of an array
  carrying character arrays. This function can be used
  with the push function for a FIFO (First In-First Out)
  data structure that stands for the std::queue
  that is availabld in C++.
  -Parameters-
  char **list - char double pointer to the list carrying
                the strings
  char *outstr- char array pointing to a pre allocated
                string where the first value of the array
                will be copied to.
  int len     - integer (global variable from the caller)

Apparently you meant curlen, not len.  

Exactly correct - typo

Furthermore, you have no idea
how the argument corresponding to this parameter is coded in the
calling function.  The assumption that it is global seems particularly
unlikely since, if it were, there would be no need to pass it as an
argument.

Yup, you're right.... I just "meant" that the variable must be
transparent between push() and pop()

                that carries the number of elements that
                list carries
  -Return value-
  integer     - the int value returned by the function
                represents the number of elements in list.
                This will basically be curlen - 1.
*******************************************************/
{
 int j=0;
 int i=0;
 char **temp;
 if (curlen==0) {
   printf("pop(): No element left in list\n");
   outstr="";
   return 0;
 }
/* WARNING!!! Why does it seem to still work fine if there's fewer
bytes allocated to outstr than list[0] is long?*/

Bad karma.  One of the worst manifestations of undefined behavior is
to appear to work as expected.

So...? What do you recommend to do here? Just to ensure outstr is big
enough to be able to hold list{0]'s content?

 strcpy(outstr, list[0]);
 for (j=1; j<curlen; j++){
   temp = realloc(list,(curlen)*sizeof(*list));

How many times do you think you need to reallocate list?  Hint: how
many items did you pop off the list?

There'll be as many pop as there were pushes - no limit

Did you mean to use curlen-1 here?  The only purpose is to reduce the
amount of dynamically allocated memory that list points to and this
allocates the exact same amount of memory.

Exactly, that's what i thought as well but if I make curlen-1 i always
end up getting a segmentation fault and i can't figure out why...

   if (temp==NULL){
     printf("pop(): Error reallocating temp memory for msglist\n");
     for (i=curlen;i>=0;i--) {
       free(temp);

Since temp is NULL, it should be obvious that temp does not exist
and attempting to evaluate it invokes undefined behavior.

       curlen--;

Why use i at all when curlen works just as well?  As it stands, this
code is useless since curlen is not used after the for loop variable
has been initialized.
hOOps...:$

     }
     free(list);

So free-ing list would be enough to clean up all the memory in case
something goes wrong on realloc?

     return -1;
   }
   temp[j-1]=malloc (strlen(list[j])+1);
   strcpy(temp[j-1],list[j]);

There is no need to move the strings themselves.  It is sufficient to
copy the pointers in list to temp.

temp[j-1] = list[j];

You have just created a massive set of memory leaks.  All the dynamic
areas previously pointed to by the members of list are now
inaccessible.

Hm? But my content (to use) is now in temp... how would I get it back
to list?

The extra code necessary to perform the side effect of the -- operator
is a waste since curlen will be destroyed as soon as the function
exits.

Exactly but it will still return the value that can be retrieved by
the caller.

}

Also, when i call the pop() function as below, what happens if "res"
isn't "long" enough?

The same undefined behavior described above.

char res[1024]={};
int Len;
Len=pop(msglist,res,Len);
Thanks for hints and suggestions!

Cut and paste; don't retype.
huh??

Thanks a lot for your help, I do appreciate you assisting me here a
little!

 

[Eric Sosman]

Hi,
I wrote two functions to push() a character array onto an array and
to
pop() the first element[0] of that array again.
The array holding the elements should be dynamically extendible. So
my
this is my code,[...]

Are you sure? Even after adding #includes for the missing
headers, gcc says

gcc -Wall -W -std=c99 -pedantic -c foo.c
foo.c: In function 'push':
foo.c:31: error: 'i' undeclared (first use in this function)
foo.c:31: error: (Each undeclared identifier is reported only once
foo.c:31: error: for each function it appears in.)
foo.c:31: error: 'Len' undeclared (first use in this function)
foo.c:32: error: 'msglist' undeclared (first use in this function)

It seems what you've shown us is a sort of rough approximation
of your code. There are lots of things wrong with it (beyond the
obvious blunders gcc finds), but I don't feel like spending time
correcting some problem other than the one you're actually having.
If you'll try again, with *complete* and *actual* code, I may be
more able to help.

Okay below i posted the whole source code from my little test app:
don't forget to compile it with the -pthread flag.
#include<stdio.h>
#include<stdlib.h>
#include<string.h>
#include<pthread.h>
int push(char**,char*, int);
int pop(char**,char*, int);
char **msglist;
int Len=0;
int i=0;
int done=0;
pthread_mutex_t log_mtx;
char* str="Novax";
char chrarr[100]={0};

//-------------------------------------------------------
int push(char **list, char *str, int curlen){
/*******************************************************
-Description-
This function allocates memory on an array for character
arrays to carry an array of strings. This function is
intended to be used with the pop function for a FIFO
(First In-First Out) data structure that stands for
the std::queue that is availabld in C++.
-Parameters-
char **list - char double pointer to the list carrying
the strings
char *str - char array pointing to the string that
should be added to the array list
int len - integer (global variable from the caller)
that carries the number of elements that
list already carries (required for offset
calculation)

Never used. Did you mean `Len'?

-Return value-
integer - the int value returned by the function
represents the number of elements in list.
This will basically be curlen + 1.

It's too bad that `curlen' isn't described ... From the
code, there seems to be a close connection between `curlen'
and `Len' (also not described), and it's not clear why you
need both of them.

It's also too bad that the use of the global variable
`msglist' isn't described ... Again, from the code there
seems to be a close connection between `list' and `msglist',
and again it's not clear why you need both.

*******************************************************/

char **temp;

temp = realloc(list,(curlen+1)*sizeof(*list));
if (temp==NULL){
printf("push(): Error reallocating memory for msglist\n");
for (i=Len;i>=0;i--) {

If the description of `len' applies to `Len', then this
loop executes once too often. (Consider the boundary case
where the queue is empty and `Len', the number of strings
already queued, is zero. How many free() calls should you
make when you need to free() no strings at all?)

free(msglist);
Len--;
}
free(list);

Strange mixture of `list' and `msglist' here.

return -1;
}
msglist=temp;
msglist[curlen]=malloc (strlen(str)+1);
strcpy(msglist[curlen],str);

You're careful to check for realloc() failure, and that's
good. Are you aware that malloc() can also fail?

return ++curlen;
}
//-------------------------------------------------------
int pop (char ** list, char *outstr, int curlen)
/*******************************************************
-Description-
This function pops the first value of an array
carrying character arrays. This function can be used
with the push function for a FIFO (First In-First Out)
data structure that stands for the std::queue
that is availabld in C++.
-Parameters-
char **list - char double pointer to the list carrying
the strings
char *outstr- char array pointing to a pre allocated
string where the first value of the array
will be copied to.
int len - integer (global variable from the caller)
that carries the number of elements that
list carries

Never used. Did you mean `Len'?

-Return value-
integer - the int value returned by the function
represents the number of elements in list.
This will basically be curlen - 1.
*******************************************************/
{
int j=0;
int i=0;
char **temp;
if (curlen==0) {
printf("pop(): No element left in list\n");
outstr="";

Useless assignment. Perhaps you want something like

outstr[0] = '\0';

return 0;
}
/* WARNING!!! Why does it seem to still work fine if there's fewer
bytes allocated to outstr than list[0] is long?*/

Because "undefined behavior" is "undefined." Also, "seem"
is only "seem."

strcpy(outstr, list[0]);

The loop that follows is mixed up beyond the power of a
few line-by-line edits to correct. You probably want something
along the lines of

free (list[0]);
for (j = 1; j < curlen; j++)
list[j-1] = list[j];
--curlen;
temp = realloc(list, curlen * sizeof *list);
if (temp == NULL) {
complain and die -- if you really think it
necessary, which it doesn't seem to be
}
msglist = temp;

The slide-em-down loop could be replaced by a single
call to memmove() if desired. Also, note again the strange
entanglement between `list' and `msglist' -- but assigning
something to `list' has no effect on the caller.

for (j=1; j<curlen; j++){
temp = realloc(list,(curlen)*sizeof(*list));
if (temp==NULL){
printf("pop(): Error reallocating temp memory for msglist\n");
for (i=curlen;i>=0;i--) {
free(temp);
curlen--;
}
free(list);
return -1;
}
temp[j-1]=malloc (strlen(list[j])+1);
strcpy(temp[j-1],list[j]);
}
list=temp;

return --curlen;
}
//-------------------------------------------------------
void _run_logger(void){
int locLen=0;
char res[5]={0};
// char *res=NULL;
int s=0;
printf("THREAD started\n");
while (1) {
pthread_mutex_lock(&log_mtx);
locLen=Len;
pthread_mutex_unlock(&log_mtx);
if (locLen>0) {
/* WARNING! What happens if more bytes get set to 0 than there's
allocated space in res? */

As with any other undefined behavior: Anything At All
can happen. In the code shown, a crash is likely -- but not
guaranteed; Anything means Anything, without restriction.

memset(res,'\0',1024);
pthread_mutex_lock(&log_mtx);
Len=pop(msglist,res,Len);

<off-topic> If you intend the mutex to protect accesses to
the shared queue, you're doing it R-O-N-G, wrong. Hint: Can
something happen to the queue between the time you check `locLen'

printf("<<popped %s off from head of msglist, newLen%d\n",res,
Len);
pthread_mutex_unlock(&log_mtx);
}
else {
printf ("Queue is empty, locLen:%d\n",locLen);
for(s=0;s<1000000;s++);
}
}

}
//-------------------------------------------------------
int main (void){

pthread_mutex_init(&log_mtx, NULL);
int rc=0;
pthread_t threads;
int o=0;

rc = pthread_create(&threads, NULL, (void*)_run_logger,0);

<off-topic> The compiler should have given you a diagnostic

if (rc){
printf("ERROR; return code from pthread_create() is %d\n", rc);
exit(-1);
}

for (i=0;i<=1000;i++){
sprintf(chrarr,"%s%dHello",str,i);
pthread_mutex_lock(&log_mtx);
Len=push(msglist, chrarr, Len);
printf(">>push %s newLen:%d\n",msglist[Len-1], Len);
pthread_mutex_unlock(&log_mtx);
for(o=0;o<100000;o++);
}
/* give the thread some time to process the leftover elements*/
for(o=0;o<10000000;o++);
return 0;
}

General comments:

- Your implementation has lots of problems, large and small.
I've pointed out a few of them, but have not made a diligent
search and may have overlooked others.

- The design is, well, awful. Global variables are eyebrow-
raisers all by themselves; global variables that prevent
you from having more than one queue in a program are worse;
global variables with mysterious (undocumented!) connections
to function parameters are bletcherous. Perhaps it's time
you gained some familiarity with structs, which provide a
way of grouping several dissimilar pieces of data into a
single, easily-managed package.

- The design choice to make copies of all those strings, both
inbound and outbound, is a strange one. Defensible, maybe,
but strange. It seems to me you'd do better to let the
caller handle the memory for the strings, using whatever
kind of allocation seems best, and let the queue just deal
with the string pointers. You could add a convenience
copy_and_push() function, if you like.

- There are more efficient ways to manage the array of
string pointers than by calling realloc() on every push
and pop. (As things stand, you're also calling malloc()
on every push, and both malloc() and free() on every pop;
see the earlier comment about doing away with all that
silly copying.)

- Many of your `int' variables could benefit by becoming
`size_t' variables instead.

- <off-topic> The attempt to handle multi-threading is
broken. The comp.programming.threads newsgroup can go
into more detail about this. </off-topic>

 

[Barry Schwarz]

Hi,
I wrote two functions to push() a character array onto an array and
to
pop() the first element[0] of that array again.
The array holding the elements should be dynamically extendible. So
my
this is my code, the push () function seems to work well but i have
questions for my pop() implementation.

snip an obviously incorrect copy of push

int pop (char ** list, char *outstr, int curlen)
/*******************************************************
  -Description-
  This function pops the first value of an array
  carrying character arrays. This function can be used
  with the push function for a FIFO (First In-First Out)
  data structure that stands for the std::queue
  that is availabld in C++.
  -Parameters-
  char **list - char double pointer to the list carrying
                the strings
  char *outstr- char array pointing to a pre allocated
                string where the first value of the array
                will be copied to.
  int len     - integer (global variable from the caller)

Apparently you meant curlen, not len.  

Exactly correct - typo

Furthermore, you have no idea
how the argument corresponding to this parameter is coded in the
calling function.  The assumption that it is global seems particularly
unlikely since, if it were, there would be no need to pass it as an
argument.

Yup, you're right.... I just "meant" that the variable must be
transparent between push() and pop()

                that carries the number of elements that
                list carries
  -Return value-
  integer     - the int value returned by the function
                represents the number of elements in list.
                This will basically be curlen - 1.
*******************************************************/
{
 int j=0;
 int i=0;
 char **temp;
 if (curlen==0) {
   printf("pop(): No element left in list\n");
   outstr="";
   return 0;
 }
/* WARNING!!! Why does it seem to still work fine if there's fewer
bytes allocated to outstr than list[0] is long?*/

Bad karma.  One of the worst manifestations of undefined behavior is
to appear to work as expected.

So...? What do you recommend to do here? Just to ensure outstr is big
enough to be able to hold list{0]'s content?

I know of no way for pop() to be able to determine this on its own.
One possible way to assist pop() would be to pass the size of whatever
outstr points to as an additional parameter. Another option is to
return the address of the popped string (no size issue since it is not
being copied) and let the calling function free it.

 strcpy(outstr, list[0]);
 for (j=1; j<curlen; j++){
   temp = realloc(list,(curlen)*sizeof(*list));

How many times do you think you need to reallocate list?  Hint: how
many items did you pop off the list?

There'll be as many pop as there were pushes - no limit

You missed the point. One call to pop() only pops one item off. You
are reallocating list curlen times each time you call pop(). curlen-1
of those reallocations are a waste.

Exactly, that's what i thought as well but if I make curlen-1 i always
end up getting a segmentation fault and i can't figure out why...

So it is better to camouflage the problem than solve it?!

   if (temp==NULL){
     printf("pop(): Error reallocating temp memory for msglist\n");
     for (i=curlen;i>=0;i--) {
       free(temp);

Since temp is NULL, it should be obvious that temp does not exist
and attempting to evaluate it invokes undefined behavior.

       curlen--;

Why use i at all when curlen works just as well?  As it stands, this
code is useless since curlen is not used after the for loop variable
has been initialized.
hOOps...:$

     }
     free(list);

So free-ing list would be enough to clean up all the memory in case
something goes wrong on realloc?

Not at all. Every pointer that contains the address of reallocated
memory must be freed individually. Your call to free is using the
wrong argument.

     return -1;
   }
   temp[j-1]=malloc (strlen(list[j])+1);
   strcpy(temp[j-1],list[j]);

There is no need to move the strings themselves.  It is sufficient to
copy the pointers in list to temp.

temp[j-1] = list[j];

You have just created a massive set of memory leaks.  All the dynamic
areas previously pointed to by the members of list are now
inaccessible.

Hm? But my content (to use) is now in temp... how would I get it back
to list?

Copies of your content is now pointed to by the members of temp. All
the original content was pointed to by the members of list and now
those members and the content they pointed to are still allocated and
unreachable.

Exactly but it will still return the value that can be retrieved by
the caller.

So what is wrong with
return curlen-1;
which doesn't waste resources with unneeded side affects.

}

Also, when i call the pop() function as below, what happens if "res"
isn't "long" enough?

The same undefined behavior described above.

char res[1024]={};
int Len;
Len=pop(msglist,res,Len);
Thanks for hints and suggestions!

Cut and paste; don't retype.

huh??

Your code for push() was obviously not what you compiled.

Please don't quote signatures (the "-- " and subsequent text).

 

[Chad]

Hi,
I wrote two functions to push() a character array onto an array and
to
pop() the first element[0] of that array again.
The array holding the elements should be dynamically extendible. So
my
this is my code, the push () function seems to work well but i have
questions for my pop() implementation.

snip an obviously incorrect copy of push

int pop (char ** list, char *outstr, int curlen)
/*******************************************************
  -Description-
  This function pops the first value of an array
  carrying character arrays. This function can be used
  with the push function for a FIFO (First In-First Out)
  data structure that stands for the std::queue
  that is availabld in C++.
  -Parameters-
  char **list - char double pointer to the list carrying
                the strings
  char *outstr- char array pointing to a pre allocated
                string where the first value of the array
                will be copied to.
  int len     - integer (global variable from the caller)

Apparently you meant curlen, not len.  Furthermore, you have no idea
how the argument corresponding to this parameter is coded in the
calling function.  The assumption that it is global seems particularly
unlikely since, if it were, there would be no need to pass it as an
argument.

                that carries the number of elements that
                list carries
  -Return value-
  integer     - the int value returned by the function
                represents the number of elements in list.
                This will basically be curlen - 1.
*******************************************************/
{
 int j=0;
 int i=0;
 char **temp;
 if (curlen==0) {
   printf("pop(): No element left in list\n");
   outstr="";
   return 0;
 }
/* WARNING!!! Why does it seem to still work fine if there's fewer
bytes allocated to outstr than list[0] is long?*/

Bad karma.  One of the worst manifestations of undefined behavior is
to appear to work as expected.

 strcpy(outstr, list[0]);
 for (j=1; j<curlen; j++){
   temp = realloc(list,(curlen)*sizeof(*list));

How many times do you think you need to reallocate list?  Hint: how
many items did you pop off the list?

Did you mean to use curlen-1 here?  The only purpose is to reduce the
amount of dynamically allocated memory that list points to and this
allocates the exact same amount of memory.

   if (temp==NULL){
     printf("pop(): Error reallocating temp memory for msglist\n");
     for (i=curlen;i>=0;i--) {
       free(temp);

Since temp is NULL, it should be obvious that temp does not exist
and attempting to evaluate it invokes undefined behavior.

       curlen--;

Why use i at all when curlen works just as well?  As it stands, this
code is useless since curlen is not used after the for loop variable
has been initialized.

How do you figure that 'curlen' is not used after the for loop
variable has been initialized? Wouldn't both 'i' and 'curlen' get
decreased in the for loop?

 

[Barry Schwarz]

Hi,
I wrote two functions to push() a character array onto an array and
to
pop() the first element[0] of that array again.
The array holding the elements should be dynamically extendible. So
my
this is my code, the push () function seems to work well but i have
questions for my pop() implementation.

snip an obviously incorrect copy of push

int pop (char ** list, char *outstr, int curlen)
/*******************************************************
  -Description-
  This function pops the first value of an array
  carrying character arrays. This function can be used
  with the push function for a FIFO (First In-First Out)
  data structure that stands for the std::queue
  that is availabld in C++.
  -Parameters-
  char **list - char double pointer to the list carrying
                the strings
  char *outstr- char array pointing to a pre allocated
                string where the first value of the array
                will be copied to.
  int len     - integer (global variable from the caller)

Apparently you meant curlen, not len.  Furthermore, you have no idea
how the argument corresponding to this parameter is coded in the
calling function.  The assumption that it is global seems particularly
unlikely since, if it were, there would be no need to pass it as an
argument.

                that carries the number of elements that
                list carries
  -Return value-
  integer     - the int value returned by the function
                represents the number of elements in list.
                This will basically be curlen - 1.
*******************************************************/
{
 int j=0;
 int i=0;
 char **temp;
 if (curlen==0) {
   printf("pop(): No element left in list\n");
   outstr="";
   return 0;
 }
/* WARNING!!! Why does it seem to still work fine if there's fewer
bytes allocated to outstr than list[0] is long?*/

Bad karma.  One of the worst manifestations of undefined behavior is
to appear to work as expected.

 strcpy(outstr, list[0]);
 for (j=1; j<curlen; j++){
   temp = realloc(list,(curlen)*sizeof(*list));

How many times do you think you need to reallocate list?  Hint: how
many items did you pop off the list?

Did you mean to use curlen-1 here?  The only purpose is to reduce the
amount of dynamically allocated memory that list points to and this
allocates the exact same amount of memory.

   if (temp==NULL){
     printf("pop(): Error reallocating temp memory for msglist\n");
     for (i=curlen;i>=0;i--) {
       free(temp);

Since temp is NULL, it should be obvious that temp does not exist
and attempting to evaluate it invokes undefined behavior.

       curlen--;

Why use i at all when curlen works just as well?  As it stands, this
code is useless since curlen is not used after the for loop variable
has been initialized.

How do you figure that 'curlen' is not used after the for loop
variable has been initialized? Wouldn't both 'i' and 'curlen' get
decreased in the for loop?

What I should have said was that the decremented value of curlen is
never used in the loop.

 

[cerr]

Hi,
I wrote two functions to push() a character array onto an array and
to
pop() the first element[0] of that array again.
The array holding the elements should be dynamically extendible. So
my
this is my code, the push () function seems to work well but i have
questions for my pop() implementation.
snip an obviously incorrect copy of push
int pop (char ** list, char *outstr, int curlen)
/*******************************************************
  -Description-
  This function pops the first value of an array
  carrying character arrays. This function can be used
  with the push function for a FIFO (First In-First Out)
  data structure that stands for the std::queue
  that is availabld in C++.
  -Parameters-
  char **list - char double pointer to the list carrying
                the strings
  char *outstr- char array pointing to a pre allocated
                string where the first value of the array
                will be copied to.
  int len     - integer (global variable from the caller)
Apparently you meant curlen, not len.  

Exactly correct - typo

Yup, you're right.... I just "meant" that the variable must be
transparent between push() and pop()

                that carries the number of elements that
                list carries
  -Return value-
  integer     - the int value returned by the function
                represents the number of elements in list.
                This will basically be curlen - 1.
*******************************************************/
{
 int j=0;
 int i=0;
 char **temp;
 if (curlen==0) {
   printf("pop(): No element left in list\n");
   outstr="";
   return 0;
 }
/* WARNING!!! Why does it seem to still work fine if there's fewer
bytes allocated to outstr than list[0] is long?*/
Bad karma.  One of the worst manifestations of undefined behavior is
to appear to work as expected.

So...? What do you recommend to do here? Just to ensure outstr is big
enough to be able to hold list{0]'s content?

I know of no way for pop() to be able to determine this on its own.
One possible way to assist pop() would be to pass the size of whatever
outstr points to as an additional parameter.  Another option is to
return the address of the popped string (no size issue since it is not
being copied) and let the calling function free it.

 strcpy(outstr, list[0]);
 for (j=1; j<curlen; j++){
   temp = realloc(list,(curlen)*sizeof(*list));
How many times do you think you need to reallocate list?  Hint: how
many items did you pop off the list?

There'll be as many pop as there were pushes - no limit

You missed the point.  One call to pop() only pops one item off.  You
are reallocating list curlen times each time you call pop().  curlen-1
of those reallocations are a waste.

Oh yeah, now I'm getting it!
But if I do a
temp = realloc(list,(curlen-1)*sizeof(*list));
before the for loop i always run into a seg fault. not immediately but
soon, curlen is not always the same but it often is around 90.. :
( That's weird... any clues here?

So it is better to camouflage the problem than solve it?!

it certainly is not but i just postponed it - which is dangerous i
agree!

   if (temp==NULL){
     printf("pop(): Error reallocating temp memory for msglist\n");
     for (i=curlen;i>=0;i--) {
       free(temp);
Since temp is NULL, it should be obvious that temp does not exist
and attempting to evaluate it invokes undefined behavior.
       curlen--;
Why use i at all when curlen works just as well?  As it stands, this
code is useless since curlen is not used after the for loop variable
has been initialized.

hOOps...:$

     }
     free(list);

So free-ing list would be enough to clean up all the memory in case
something goes wrong on realloc?

Not at all.  Every pointer that contains the address of reallocated
memory must be freed individually.  Your call to free is using the
wrong argument.

huh? so i need to run through something like this then, right?:

if (temp==NULL){
printf("pop(): Error reallocating temp memory for msglist\n");
for (curlen=curlen;curlen>=0;curlen--)
free(temp[curlen]);
free(temp);
return -1;
}

     return -1;
   }
   temp[j-1]=malloc (strlen(list[j])+1);
   strcpy(temp[j-1],list[j]);
There is no need to move the strings themselves.  It is sufficient to
copy the pointers in list to temp.

temp[j-1] = list[j];

 }
 list=temp;
You have just created a massive set of memory leaks.  All the dynamic
areas previously pointed to by the members of list are now
inaccessible.

Hm? But my content (to use) is now in temp... how would I get it back
to list?

Copies of your content is now pointed to by the members of temp.  All
the original content was pointed to by the members of list and now
those members and the content they pointed to are still allocated and
unreachable.

Uh okay, So i would need to make a
temp[j-1] = list[j];
free(list[j]);
correct?

Your code for push() was obviously not what you compiled.

int push(char **list, char *str, int curlen){

char **temp;
int i=0;

temp = realloc(list,(curlen+1)*sizeof(*list));
if (temp==NULL){
printf("push(): Error reallocating memory for msglist\n");
for (i=Len;i>=0;i--) {
free(msglist);
Len--;
}
free(list);
return -1;
}
msglist=temp;
msglist[curlen]=malloc (strlen(str)+1);
strcpy(msglist[curlen],str);

return ++curlen;
}

Thanks for your help Barry! It is greatly appreciated!

 

[Barry Schwarz]

This does not do what you think it does. The value in outstr is never
"returned" to the calling function.

   return 0;
 } snip
 strcpy(outstr, list[0]);
 for (j=1; j<curlen; j++){
   temp = realloc(list,(curlen)*sizeof(*list));

How many times do you think you need to reallocate list?  Hint: how
many items did you pop off the list?

There'll be as many pop as there were pushes - no limit

You missed the point.  One call to pop() only pops one item off.  You
are reallocating list curlen times each time you call pop().  curlen-1
of those reallocations are a waste.

Oh yeah, now I'm getting it!
But if I do a
temp = realloc(list,(curlen-1)*sizeof(*list));
before the for loop i always run into a seg fault. not immediately but
soon, curlen is not always the same but it often is around 90.. :
( That's weird... any clues here?

You have a major misunderstanding of what realloc does.

snip

   if (temp==NULL){
     printf("pop(): Error reallocating temp memory for msglist\n");
     for (i=curlen;i>=0;i--) {
       free(temp); snip
       curlen--; snip
     }
     free(list);

So free-ing list would be enough to clean up all the memory in case
something goes wrong on realloc?

Not at all.  Every pointer that contains the address of reallocated
memory must be freed individually.  Your call to free is using the
wrong argument.

huh? so i need to run through something like this then, right?:

if (temp==NULL){
printf("pop(): Error reallocating temp memory for msglist\n");
for (curlen=curlen;curlen>=0;curlen--)

Does the expression curlen=curlen do something meaningful.?

free(temp[curlen]);

Same problem. If temp is NULL, temp[anything] does not exist.

free(temp);

Freeing a NULL value is legal but pointless.

Look in your reference and see what realloc does if it cannot satisfy
the request.

return -1;
}

     return -1;
   }
   temp[j-1]=malloc (strlen(list[j])+1);

Look in your reference and see what realloc does when it succeeds.
What happens to the value in list? What happens to the values that
were in the "old" allocated memory?

   strcpy(temp[j-1],list[j]);

There is no need to move the strings themselves.  It is sufficient to
copy the pointers in list to temp.

This is my error. When you understand what realloc does, you will
know what I should have said here.

temp[j-1] = list[j];

You will also understand why this invokes undefined behavior.

This would have been true if I my comment above had not been in error.

You still need to research realloc.

Part of the same error on my part.

Uh okay, So i would need to make a
temp[j-1] = list[j];
free(list[j]);
correct?

After you research realloc, you will understand why the answer is no.

int push(char **list, char *str, int curlen){

char **temp;
int i=0;

temp = realloc(list,(curlen+1)*sizeof(*list));
if (temp==NULL){
printf("push(): Error reallocating memory for msglist\n");
for (i=Len;i>=0;i--) {

And Len is defined where? Surely you are not trying to execute code
that doesn't compile cleanly.

free(msglist);

msglist is defined where?

Len--;
}
free(list);
return -1;
}
msglist=temp;
msglist[curlen]=malloc (strlen(str)+1);
strcpy(msglist[curlen],str);

return ++curlen;
}

Alternately, you are not compiling the code at all.

 

[cerr]

This does not do what you think it does.  The value in outstr is never
"returned" to the calling function.

Yes, I just realized, I would need to use a char **outstr and then go
like (*outstr)=""; While I would call the function using &str (if str
is declared as a char*).

return 0;
} snip
strcpy(outstr, list[0]);
for (j=1; j<curlen; j++){
temp = realloc(list,(curlen)*sizeof(*list));
How many times do you think you need to reallocate list? Hint: how
many items did you pop off the list?
There'll be as many pop as there were pushes - no limit
You missed the point. One call to pop() only pops one item off. You
are reallocating list curlen times each time you call pop(). curlen-1
of those reallocations are a waste.

Oh yeah, now I'm getting it!
But if I do a
temp = realloc(list,(curlen-1)*sizeof(*list));
before the for loop i always run into a seg fault. not immediately but
soon, curlen is not always the same but it often is around 90.. :
( That's weird... any clues here?

You have a major misunderstanding of what realloc does.

Yeah, I probably do...
void * realloc ( void * ptr, size_t size );
The size of the memory block pointed to by the ptr parameter is
changed to the size bytes, expanding or reducing the amount of memory
available in the block.
So okay, I'm changing the size of list and then assign that same
address to temp so I can copy the content from list to temp and minus
the first element.
Hold on,
Do you think following is better:

temp=malloc((curlen-1)*sizeof(*list));
for (j=1; j<curlen; j++){
temp[j-1] = malloc (strlen(list[j])+1);
temp[j-1] = list[j];
}
realloc(list,(curlen-1)*sizeof(*list));
list=temp;
for (j=1; j<curlen; j++)
free(temp[j]);
free(temp);

snip

if (temp==NULL){
printf("pop(): Error reallocating temp memory for msglist\n");
for (i=curlen;i>=0;i--) {
free(temp); snip
curlen--; snip
}
free(list);
So free-ing list would be enough to clean up all the memory in case
something goes wrong on realloc?
Not at all. Every pointer that contains the address of reallocated
memory must be freed individually. Your call to free is using the
wrong argument.

huh? so i need to run through something like this then, right?:

   if (temp==NULL){
     printf("pop(): Error reallocating temp memory for msglist\n");
     for (curlen=curlen;curlen>=0;curlen--)

Does the expression curlen=curlen do something meaningful.?

Nope but it's just the first argument in the for loop.

       free(temp[curlen]);

Same problem.  If temp is NULL, temp[anything] does not exist.

If above code makes sense this would be omitted...

     free(temp);

Freeing a NULL value is legal but pointless.

Look in your reference and see what realloc does if it cannot satisfy
the request.

     return -1;
   }

return -1;
}
temp[j-1]=malloc (strlen(list[j])+1);

Look in your reference and see what realloc does when it succeeds.
What happens to the value in  list?  What happens to the values that
were in the "old" allocated memory?

strcpy(temp[j-1],list[j]);
There is no need to move the strings themselves. It is sufficient to
copy the pointers in list to temp.

This is my error.  When you understand what realloc does, you will
know what I should have said here.

Realloc does change the allocated memory area, and if i get to use
above code, the temp would be created with one element less, and then
the content would be copied to temp where it resides until the whole
bnunch of pointers get copied back to list. No that won't work either
because it's just addresses that get copied. Do I instead go like
memcpy(list,temp,(curlen-1)*sizeof(*list)); would that work better?

temp[j-1] = list[j];

You will also understand why this invokes undefined behavior.



This would have been true if I my comment above had not been in error.



You still need to research realloc.



Part of the same error on my part.

Uh okay, So i would need to make a
temp[j-1] = list[j];
free(list[j]);
correct?

After you research realloc, you will understand why the answer is no. [snip]

 if (temp==NULL){
   printf("push(): Error reallocating memory for msglist\n");
   for (i=Len;i>=0;i--) {

And Len is defined where?  Surely you are not trying to execute code
that doesn't compile cleanly.

Yup, a typo you pointed out there, this should have been curlen.

     free(msglist);

msglist is defined where?

that's another typo, this should be list. but i realize that i can't
use list instead of msglist...uuuhm...i think i need a triple pointer
here...

Okay, I changed the code around and have this now:
int push(char ***list, char *str, int curlen){
char **temp;
int i=0;

temp = realloc((*list),(curlen+1)*sizeof(*list));
if (temp==NULL){
printf("push(): Error reallocating memory for msglist\n");
for (i=curlen;i>=0;i--) {
free((*list));
QLen--;
}
free(list);
return -1;
}
(*list)=temp;
(*list)[curlen]=malloc (strlen(str)+1);
strcpy((*list)[curlen],str);

return ++curlen;
}

 

[cerr]

This does not do what you think it does.  The value in outstr is never
"returned" to the calling function.

Yes, I just realized, I would need to use a char **outstr and then go
like (*outstr)=""; While I would call the function using &str (if str
is declared as a char*).

return 0;
} snip
strcpy(outstr, list[0]);
for (j=1; j<curlen; j++){
temp = realloc(list,(curlen)*sizeof(*list));
How many times do you think you need to reallocate list? Hint: how
many items did you pop off the list?
There'll be as many pop as there were pushes - no limit
You missed the point. One call to pop() only pops one item off. You
are reallocating list curlen times each time you call pop(). curlen-1
of those reallocations are a waste.
Oh yeah, now I'm getting it!
But if I do a
temp = realloc(list,(curlen-1)*sizeof(*list));
before the for loop i always run into a seg fault. not immediately but
soon, curlen is not always the same but it often is around 90.. :
( That's weird... any clues here?

You have a major misunderstanding of what realloc does.

Yeah, I probably do...
void * realloc ( void * ptr, size_t size );
The size of the memory block pointed to by the ptr parameter is
changed to the size bytes, expanding or reducing the amount of memory
available in the block.
So okay, I'm changing the size of list and then assign that same
address to temp so I can copy the content from list to temp and minus
the first element.
Hold on,
Do you think following is better:

temp=malloc((curlen-1)*sizeof(*list));
for (j=1; j<curlen; j++){
    temp[j-1] = malloc (strlen(list[j])+1);
    temp[j-1] = list[j];}

realloc(list,(curlen-1)*sizeof(*list));
list=temp;
for (j=1; j<curlen; j++)
 free(temp[j]);
free(temp);

snip

if (temp==NULL){
printf("pop(): Error reallocating temp memory for msglist\n");
for (i=curlen;i>=0;i--) {
free(temp); snip

curlen--;

snip
}
free(list);
So free-ing list would be enough to clean up all the memory in case
something goes wrong on realloc?
Not at all. Every pointer that contains the address of reallocated
memory must be freed individually. Your call to free is using the
wrong argument.
huh? so i need to run through something like this then, right?:
   if (temp==NULL){
     printf("pop(): Error reallocating temp memory for msglist\n");
     for (curlen=curlen;curlen>=0;curlen--)

Does the expression curlen=curlen do something meaningful.?

Nope but it's just the first argument in the for loop.

       free(temp[curlen]);

Same problem.  If temp is NULL, temp[anything] does not exist.

If above code makes sense this would be omitted...

Freeing a NULL value is legal but pointless.

Look in your reference and see what realloc does if it cannot satisfy
the request.

     return -1;
   }
return -1;
}
temp[j-1]=malloc (strlen(list[j])+1);

Look in your reference and see what realloc does when it succeeds.
What happens to the value in  list?  What happens to the values that
were in the "old" allocated memory?

strcpy(temp[j-1],list[j]);
There is no need to move the strings themselves. It is sufficient to
copy the pointers in list to temp.

This is my error.  When you understand what realloc does, you will
know what I should have said here.

Realloc does change the allocated memory area, and if i get to use
above code, the temp would be created with one element less, and then
the content would be copied to temp where it resides until the whole
bnunch of pointers get copied back to list. No that won't work either
because it's just addresses that get copied. Do I instead go like
memcpy(list,temp,(curlen-1)*sizeof(*list)); would that work better?

temp[j-1] = list[j];

You will also understand why this invokes undefined behavior.

This would have been true if I my comment above had not been in error.

You still need to research realloc.

Part of the same error on my part.

Uh okay, So i would need to make a
temp[j-1] = list[j];
free(list[j]);
correct?

After you research realloc, you will understand why the answer is no. [snip]

 if (temp==NULL){
   printf("push(): Error reallocating memory for msglist\n");
   for (i=Len;i>=0;i--) {

And Len is defined where?  Surely you are not trying to execute code
that doesn't compile cleanly.

Yup, a typo you pointed out there, this should have been curlen.

     free(msglist);

msglist is defined where?

that's another typo, this should be list. but i realize that i can't
use list instead of msglist...uuuhm...i think i need a triple pointer
here...

Okay, I changed the code around and have this now:
int push(char ***list, char *str, int curlen){
  char **temp;
  int i=0;

  temp = realloc((*list),(curlen+1)*sizeof(*list));
  if (temp==NULL){
    printf("push(): Error reallocating memory for msglist\n");
    for (i=curlen;i>=0;i--) {
      free((*list));
      QLen--;
    }
    free(list);
    return -1;
  }
  (*list)=temp;
  (*list)[curlen]=malloc (strlen(str)+1);
  strcpy((*list)[curlen],str);

  return ++curlen;



}

Heh,

Look what I got from a forum:
char *pop(char ***list, int *size) {
if(!*size) return 0;

char *first = (*list)[0];

memmove(*list, *list + 1, (*size) * sizeof(**list));
(*size) --;
(*list) = realloc(*list, (*size) * sizeof(**list));

return first;
}
That's way easier and works well, any drawbacks? I don't think so... I
sure learned a lot now... need to go digest a little Thanks a lot
buddy for sticking around here!

 

[Barry Schwarz]

Yes, I just realized, I would need to use a char **outstr and then go
like (*outstr)=""; While I would call the function using &str (if str
is declared as a char*).

Almost. What this does is cause str in the calling program to point
to a different area in memory. If it originally pointed to an
allocated area, it no longer does so and you have a memory leak. Even
worse, if the calling function thinks it can still use this area
(str[0] = 'a', you have undefined behavior because this will attempt
to modify a string literal.

Further more, it prevents str form being defined as an array.

What you want is
strcpy(outstr, "")
with no change to the calling function.

return 0;
} snip
strcpy(outstr, list[0]);
for (j=1; j<curlen; j++){
temp = realloc(list,(curlen)*sizeof(*list));

How many times do you think you need to reallocate list? Hint: how
many items did you pop off the list?

There'll be as many pop as there were pushes - no limit

You missed the point. One call to pop() only pops one item off. You
are reallocating list curlen times each time you call pop(). curlen-1
of those reallocations are a waste.

Oh yeah, now I'm getting it!
But if I do a
temp = realloc(list,(curlen-1)*sizeof(*list));
before the for loop i always run into a seg fault. not immediately but
soon, curlen is not always the same but it often is around 90.. :
( That's weird... any clues here?

You have a major misunderstanding of what realloc does.

Yeah, I probably do...
void * realloc ( void * ptr, size_t size );
The size of the memory block pointed to by the ptr parameter is
changed to the size bytes, expanding or reducing the amount of memory
available in the block.

That describes only HALF of what realloc does and is not particularly
correct about that. There is no guarantee that the new memory will
occupy the same location as the original.

1 - It is entirely possible that the memory pointed to by ptr
is released, as if by free() and realloc returns a pointer to a new
memory area.

2 - If realloc fails, the memory pointed to by ptr is
unchanged.

Now research the other half of what realloc does and see why you are
getting a seg fault. Hint: you are calling realloc at the wrong time.

So okay, I'm changing the size of list and then assign that same
address to temp so I can copy the content from list to temp and minus
the first element.
Hold on,
Do you think following is better:

temp=malloc((curlen-1)*sizeof(*list));

This will avoid the problems caused by your misunderstanding of
realloc. It would be easier if you just learned it right first.

for (j=1; j<curlen; j++){
temp[j-1] = malloc (strlen(list[j])+1);
temp[j-1] = list[j];

Setting a variable to one value and then immediately setting to
another independent value is always a mistake. You probably meant to
use strcpy here. That would be correct if you needed the malloc.
Since you don't, if you delete that then this assignment does what you
want.

}
realloc(list,(curlen-1)*sizeof(*list));

Surely you meant temp = realloc... here

list=temp;
for (j=1; j<curlen; j++)
free(temp[j]);
free(temp);

Once you have done this, the value of list is indeterminate. You are
also destroying everything you worked for in the previous for loop.

You need to stop coding and start thinking. Take it in individual
steps. What is the first thing you need to do after popping the
element off the array? Hint: it has nothing to do with allocation. If
you get this step, I think the rest will fall into place for you.

snip

if (temp==NULL){
printf("pop(): Error reallocating temp memory for msglist\n");
for (i=curlen;i>=0;i--) {
free(temp); snip

curlen--; snip
}
free(list);

So free-ing list would be enough to clean up all the memory in case
something goes wrong on realloc?

Not at all. Every pointer that contains the address of reallocated
memory must be freed individually. Your call to free is using the
wrong argument.

huh? so i need to run through something like this then, right?:

   if (temp==NULL){
     printf("pop(): Error reallocating temp memory for msglist\n");
     for (curlen=curlen;curlen>=0;curlen--)

Does the expression curlen=curlen do something meaningful.?

Nope but it's just the first argument in the for loop.

So is
for(; curlen >= 0; ...

       free(temp[curlen]);

Same problem.  If temp is NULL, temp[anything] does not exist.

If above code makes sense this would be omitted...

     free(temp);

Freeing a NULL value is legal but pointless.

Look in your reference and see what realloc does if it cannot satisfy
the request.

     return -1;
   }

return -1;
}
temp[j-1]=malloc (strlen(list[j])+1);

Look in your reference and see what realloc does when it succeeds.
What happens to the value in  list?  What happens to the values that
were in the "old" allocated memory?

strcpy(temp[j-1],list[j]);

There is no need to move the strings themselves. It is sufficient to
copy the pointers in list to temp.

This is my error.  When you understand what realloc does, you will
know what I should have said here.

Realloc does change the allocated memory area, and if i get to use
above code, the temp would be created with one element less, and then
the content would be copied to temp where it resides until the whole
bnunch of pointers get copied back to list. No that won't work either
because it's just addresses that get copied. Do I instead go like
memcpy(list,temp,(curlen-1)*sizeof(*list)); would that work better?

You are doing a very good job of channelling Bill Cunningham but it is
not really a desirable goal.