«

On 10/24/2011 10:42 AM, arnuld wrote:
>>> On Mon, 24 Oct 2011 14:16:30 +0100, BartC wrote:
>
>
>> struct myStack* s;
>>
>> Even disregarding myStack only having four bytes or so, I wasn't sure
>> why this struct wasn't just statically allocated. Assuming you are only
>> going to have a small number of distinct stacks.
>
> Even after working for 2 years in C (not exclusively) my brain is still
> clunky when it comes to C basics. This struct definition:
>
>
> struct myStack
> {
> int top;
> };
>
>
> can be allocated statically ?

An object of any complete type can be allocated statically. 'struct
myStack' is a complete type.

> ... I don't have the exact code (function
> definition of initialize is 100% same as the code I posted) but I tried
> using:
>
> struct myStack s;
> initialize(&s);
>
> And all I got was garbage value in s.top.

Your definition of initialize was:

void initialize(struct myStack s)
{
s.top = 0;
}

With that definition, the expression initialize(&s) is a constraint
violation, because &s has the type 'struct myStack*', which is not
compatible with 'struct myStack', which is the type that initialize was
expecting for its argument. A conforming compiler is required to issue a
diagnostic message. Did yours?

However, even if you avoided that problem, your code still won't do what
you want it to do. I'm going to change your definition slightly, in
order to make it easier to explain what's wrong with it:

void initialize(struct myStack t)
{
t.top = 0;
}

With that definition, initialize(s) is perfectly valid code. It results
in the creation of an object named t, which is initialized by copying
the current value of s. Then t.top is set to 0. Then t is discarded. s
remains unchanged.

Here's what you should have done:

void initialize(struct myStack *ps)
{
ps->top = 0;
}

With that definition, initialize(&s) is perfectly valid code. &s
produces a pointer value pointing at s. Initialize sets aside memory to
store the pointer parameter ps, and initializes it with the value of &s.
It then goes to the struct myStack object pointed at by that pointer,
and sets the 'top' member to 0. In other words, it's equivalent to
(&s)->top = 0, or s.top = 0, which is what you want.

On 10/24/2011 10:42 AM, arnuld wrote:
>>> On Mon, 24 Oct 2011 14:16:30 +0100, BartC wrote:
>
>
>> struct myStack* s;
>>
>> Even disregarding myStack only having four bytes or so, I wasn't sure
>> why this struct wasn't just statically allocated. Assuming you are only
>> going to have a small number of distinct stacks.
>
> Even after working for 2 years in C (not exclusively) my brain is still
> clunky when it comes to C basics. This struct definition:
>
>
> struct myStack
> {
> int top;
> };
>
>
> can be allocated statically ?

An object of any complete type can be allocated statically. 'struct
myStack' is a complete type.

> ... I don't have the exact code (function
> definition of initialize is 100% same as the code I posted) but I tried
> using:
>
> struct myStack s;
> initialize(&s);
>
> And all I got was garbage value in s.top.

Your definition of initialize was:

void initialize(struct myStack s)
{
s.top = 0;
}

With that definition, the expression initialize(&s) is a constraint
violation, because &s has the type 'struct myStack*', which is not
compatible with 'struct myStack', which is the type that initialize was
expecting for its argument. A conforming compiler is required to issue a
diagnostic message. Did yours?

However, even if you avoided that problem, your code still won't do what
you want it to do. I'm going to change your definition slightly, in
order to make it easier to explain what's wrong with it:

void initialize(struct myStack t)
{
t.top = 0;
}

With that definition, initialize(s) is perfectly valid code. It results
in the creation of an object named t, which is initialized by copying
the current value of s. Then t.top is set to 0. Then t is discarded. s
remains unchanged.

Here's what you should have done:

void initialize(struct myStack *ps)
{
ps->top = 0;
}

With that definition, initialize(&s) is perfectly valid code. &s
produces a pointer value pointing at s. Initialize sets aside memory to
store the pointer parameter ps, and initializes it with the value of &s.
It then goes to the struct myStack object pointed at by that pointer,
and sets the 'top' member to 0. In other words, it's equivalent to
(&s)->top = 0, or s.top = 0, which is what you want.

"arnuld" <> wrote in message
news:4ea5795f$0$287$...
>> > On Mon, 24 Oct 2011 14:16:30 +0100, BartC wrote:

> Even after working for 2 years in C (not exclusively) my brain is still
> clunky when it comes to C basics. This struct definition:
>
>
> struct myStack
> {
> int top;
> };
>
>
> can be allocated statically ?

Yes. Write:

struct myStack s;

initialise(&s);

This function then simplifies to:

void initialise(myStack *s) { s->top = 0;}

But everywhere you used s before, you'll have to write &s. In practice, code
will be in functions, that will be passed myStack objects via pointers (as
in initialise() above), so you won't need & except in a few places.

Or you can do this:

struct myStack s0 = {0};
struct myStack *s = &s0;

initialise(s); /* Although not really needed anymore */

If you still want to keep myStack objects on the heap as you'd intended,
then it might be easier to write like this:

struct myStack *s;

s=createstack();

Which function might look like this, avoiding those double-pointers:

struct myStack* createstack(void) {
struct myStack *t;

t=malloc(sizeof struct myStack);
.... /* checks */
t->top = 0;
return t;
}

(I'm not fond of having to write 'struct' everywhere, so I would probably do
this:

typedef struct {int top;} myStack;

myStack s;

which saves some typing, and hides the fact that a stack object is described
with a struct.)

--
Bartc

"BartC" <> writes:
<snip>
> struct myStack s;
>
> initialise(&s);
>
> This function then simplifies to:
>
> void initialise(myStack *s) { s->top = 0;}
>
> But everywhere you used s before, you'll have to write &s. In practice, code
> will be in functions, that will be passed myStack objects via pointers (as
> in initialise() above), so you won't need & except in a few places.
>
> Or you can do this:
>
> struct myStack s0 = {0};
> struct myStack *s = &s0;
>
> initialise(s); /* Although not really needed anymore */

Another method is to define a one-element array:

struct myStack s[1] = {0};

and now you can use s rather than &s everywhere. I am not a fan of
this, but I've found it useful to know because other people do it.

Sometime (it's worse, I think) the array-ness is hidden in a typedef:

typedef struct myStack Stack[1];
// ...
Stack s = {0};
do_something(s, ...);

<snip>
--
Ben.

I'll point out a few obvious bugs:

>#include <stdio.h>
>#include <stdlib.h>
>#include <string.h>
>
>enum { VAL_FALSE = 0, VAL_TRUE = 1, SIZE_STACK = 10 };

I guess this is legal, but a little confusing to have constants from
different contexts defined in the same enum. Simpler to do:

#define false 0
#define true 1
#define SIZE_STACK 10

>struct myStack
>{
> int top;
>};

OK, but why not collect the actual stack and stack pointer together
in one place:

struct myStack {
int top;
struct myStruct *sof[SIZE_STACK+1];
};

>void initialize(struct myStack s);
>int stackEmpty(struct myStack s);
>void push(struct myStruct* arr[], struct myStack s, const char* ele);
>
>
>int main(void)
>{
> struct myStruct* sof[SIZE_STACK+1] = {0};
> struct myStack s;
>
> initialize(s);

> stackEmpty(s);

stackEmpty() has no side effects and you're ignoring its return
value, so why call it at all?

> push(sof, s, "CLC");

Here's a problem. s is a struct, and you're passing it
by value into push(). It's technically legal, but poor form, as
I'll point out a little further down. Should really have been

push(sof, &s, "CLC");

>
> return 0;
>}
>
>


>void initialize(struct myStack s)
>{
> s.top = 0;
>}

Here's your first bug. You passed s by value, then modified
it. But you only modified a copy. The original is still
untouched, which means it's still uninitialized.

Should have passed by reference instead:

initialize(&s);
...
void initialize(struct myStack *s)
{
s->top = 0;
}

>void push(struct myStruct* arr[], struct myStack s, const char* ele)

Second bug. Again, you're modifying a local copy of s, leaving the
original unchanged. Should have been:

void push (struct myStruct* arr[], struct myStack *s, const char *ele)

with all references to "s.top" changed to "s->top".

Finally, it would be better if push() returned a boolean value
indicating the the push had failed. Error messages are good,
but program detecting problems is better.

>{
> if(NULL == arr || NULL == ele)
> {
> fprintf(stderr, "IN: %s @%d: Invalid Args\n", __FILE__, __LINE__);
> }
> else if(SIZE_STACK <= s.top)
> {
> printf("Stack Full, Can not add anymore elements\n");
> }
> else
> {
> struct myStruct* p = malloc( 1 * sizeof *p);
> if(NULL == p)
> {
> fprintf(stderr, "IN: %s @%d: Out of Memory\n", __FILE__,
>__LINE__);
> }
> else
> {
> strcpy(p->title, ele);

Third bug. Here's your segfault. You're copying the data pointed to
by ele into the space pointed to by p->title, but you never allocated
any such space. p->title is uninitialized and the strcpy() is pretty
much guaranteed to fault.

Better would be to do:

p->title = strdup(ele);

plus a check to make sure that p->title isn't NULL.

> arr[s.top] = p;
> s.top += 1;
> }
> }
>}



Try this on for size:

#define true 1
#define false 0
typedef bool int;

#define SIZE_STACE 10

typedef struct {
int top;
struct myStruct *stack[SIZE_STACK];
} MyStack;

void
initialize(MyStack *s)
{
s->top = 0;
}

bool
push(MyStack *s, const char *ele)
{
struct myStruct *p;
if (s == NULL || ele == NULL) {
fprintf(stderr, "invalid pointer, yada yada\n");
return false;
}
if (s->top >= SIZE_STACK) {
fprintf(stderr, "full stack, yada yada\n");
return false;
}
if ((p = malloc(sizeof(*p))) == NULL ||
(p->title = strdup(ele)) = NULL)
{
fprintf(stderr, "out of memory, yada yada\n");
return false;
}
s->stack[s->top++] = p;
return true;
}


There are other tweaks and improvements you could make, but they're
not imporant.
--
-Ed Falk,
http://thespamdiaries.blogspot.com/

arnuld <> writes:
>> On Mon, 24 Oct 2011 09:16:32 +0000, Ike Naar wrote:
>>> On 2011-10-24, arnuld <> wrote:
>>> else
>>> {
>>> struct myStruct* p = malloc( 1 * sizeof *p);
>
>> This allocates memory for a myStruct, but the contents of that memory is
>> still uninitialized; In particular, p->title is still an uninitialized
>> pointer.
>
> I already know this.
>
>
>>> else
>>> {
>>> strcpy(p->title, ele);
>
>> p->title is still an uninitialized pointer.
>
> How does it matter to strcpy ? strpcy(dest, src) will overwrite all the
> characters, whether its garbage or not.

strcpy() will overwrite the characters *that p->title points to*.

p->title doesn't point to garbage. p->title *is* garbage.

Remember that arguments are always passed by reference, so you're
passing the garbage *value* of p->title to strcpy.

> 'src' does contain '\0' in the
> (as its a character string), which will be written to ..... Oh.. wait a
> minute.. I got it. p->title must be an array rather than a dangling
> pointer.

No, p->title is a pointer, not an array. It needs to point to the first
element of an array object.

--
Keith Thompson (The_Other_Keith) kst- <http://www.ghoti.net/~kst>
"We must do something. This is something. Therefore, we must do this."
-- Antony Jay and Jonathan Lynn, "Yes Minister"

Azazel <> writes:
[...]
> And in C99, one can do this:
>
> struct myStack *s = &(struct myStack) { 0 };
>
> which I have found useful once or twice.

Remember that the lifetime of the object created by a compound literal
(if it's inside a function definition) is the nearest enclosing block.
(This is quite unlike the behavior of string literals.)

--
Keith Thompson (The_Other_Keith) kst- <http://www.ghoti.net/~kst>
"We must do something. This is something. Therefore, we must do this."
-- Antony Jay and Jonathan Lynn, "Yes Minister"