here is some linked list code, it segfaults after the printf of hello,
any help would be greatly appreciated.

#include <stdio.h>

struct name_layout {
char name[25];
struct name_layout *next;
};
void add_node(struct name_layout first);
void print_nodes(struct name_layout first);
void free_nodes(struct name_layout first);

struct name_layout *current;
struct name_layout first;
int main(void) {
first.next = NULL;
add_node(first);
print_nodes(first);
free_nodes(first);
return 0;
}
void add_node(struct name_layout first) {
current = &first;
while(current->next != NULL) {
current = current->next;
}
printf("Enter the name of the student: \n");
fgets(current->name, sizeof (current->name), stdin);
current->next = (struct name_layout *)malloc(sizeof (struct
name_layout));
}
void print_nodes(struct name_layout first) {
current = &first;
printf("hello\n");
while(current->next != NULL) {
printf("Students name: %s.\n", current->name);
current = current->next;
}
}
void free_nodes(struct name_layout first) {
struct name_layout *prev;

while(&first != NULL) {
current = &first;
while(current->next != NULL) {
prev = current;
current = current->next;
}
free(current);
}
}

"chrismiceli" <chrismiceli@excite.com> wrote in message
pass a pointer instead.

How about a comment to tell us what this function does?
set the next pointer to NULL and the name to the empty string.
This is also rather poor design since you are adding an empty node.
are passing printf() an uninitialised string.
Why not write your free routine recursively? This is much neater. If the
next pinter is NULL, free and return, otherwise recuse and then free.

I have cleaned up the code to meet most of your requirements, but I am
having trouble with teh add_node function, any help is greatly
appreciated.

#include <stdio.h>

struct name_layout {
char name[25];
struct name_layout *next;
};
void add_node(struct name_layout *first);
void print_nodes(struct name_layout *first);
void free_nodes(struct name_layout *node);

int main(void) {
struct name_layout first;
add_node(&first); /* Adds a node to the end of the linked
list. */
print_nodes(&first); /* Prints out the list from beginning to
end. */
free_nodes(&first); /* Free()s the entire list. */
return 0;
}
void add_node(struct name_layout *first) {
/* What goes here */
}
void print_nodes(struct name_layout *first) {
struct name_layout *current;
current = first;
printf("hello\n");
while(current->next != NULL) {
printf("Students name: %s.\n", current->name);
current = current->next;
}
}
void free_nodes(struct name_layout *node) {
struct name_layout *temp;
while(node->next != NULL) {
temp = node->next;
free(node);
free_nodes(temp);
}
}

chrismiceli wrote:

#include <stdlib.h> /* you'll need this one too :-) */

Consider using a #define for this 25.

We might need to change these...

By making the storage for the first node auto, whilst subsequent nodes are
static, you make your cleanup unnecessarily complicated.

Also, adding a node to a linked list oughtn't to include the task of
acquiring data for that node - the data should be passed in.

See below.

This won't display the name of the last student in the list!

Recursion is a lovely and wonderful thing, but here it's a bit unnecessary,
and it could end up blowing your stack (for a very long list).



Consider the following (tested) code as a model when fixing your own code.


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

#define NAME_LEN 25

struct name_layout {
char name[NAME_LEN];
struct name_layout *next;
};
void add_node(struct name_layout **first, const char *name);
void print_nodes(struct name_layout *first);
void free_nodes(struct name_layout **node);

int main(void) {
struct name_layout *first = NULL;
add_node(&first, "Peter"); /* Adds a node to
the end of the
linked list. */
add_node(&first, "Paul");
add_node(&first, "Patrick");

print_nodes(first); /* Prints out the list
from beginning to end. */

free_nodes(&first); /* Free()s the entire list. */

return 0;
}
void add_node(struct name_layout **first, const char *name) {

/* Note, in the next line, that there is no cast on malloc. In C, it's not
necessary. Also note the use of the identifier 'new'. If you're using a C++
compiler by mistake, this line is more or less guaranteed to warn you of
the fact! :-) */

struct name_layout *new = malloc(sizeof *new);
if(new != NULL)
{
/* better check we can fit the name into the space available */
if(strlen(name) < sizeof new->name)
{
strcpy(new->name, name);
new->next = NULL;

if(*first == NULL)
{
*first = new;
}
else
{
/* need to find the end of the list */
struct name_layout *tail = *first;
while(tail->next != NULL)
{
tail = tail->next;
}

/* now we can add */
tail->next = new;
}
}
else
{
fprintf(stderr, "Name too long!\n");
}
}
else
{
fprintf(stderr, "Out of memory.\n");
}
}

void print_nodes(struct name_layout *first) {
struct name_layout *current = first;

printf("hello\n");
while(current != NULL) {
printf("Students name: %s.\n", current->name);
current = current->next;
}
}
void free_nodes(struct name_layout **node) {
struct name_layout *this = *node;
struct name_layout *temp;
while(this != NULL) {
temp = this->next;
free(this);
this = temp;
}
*node = NULL;
}


--
Richard Heathfield : binary@eton.powernet.co.uk
"Usenet is a strange place." - Dennis M Ritchie, 29 July 1999.
C FAQ: http://www.eskimo.com/~scs/C-faq/top.html
K&R answers, C books, etc: http://users.powernet.co.uk/eton

"Richard Heathfield" <dontmail@address.co.uk.invalid> wrote in

/*
note this won't automatically set your pointer to zero, unlike the
non-recursive function.
*/
void free_nodes(struct name_layout *node)
{
if(node->next)
free_nodes(node->next);
free(node);
}

However Richard's code will be faster, and there is the risk of overflowing
the stack if the list is long and the stack is small.

"Malcolm" <malcolm@55bank.freeserve.co.uk> wrote in message news:c0nsom$4jn$1@news7.svr.pol.co.uk...
Call me a nitpicker, but I prefer this (if it has to be done recursively) --

void free_nodes(struct name_layout *node) {
if (node)
free_nodes(node->next);
free(node);
}

kind regards,

Jos

"Jos A. Horsmeier" <j.a.horsmeier@wanadoo.nl> wrote in message

"chrismiceli" <chrismiceli@excite.com> wrote in message
news:3751a0cb.0402141523.4c74b4ab@posting.google.c om...

You get a segfault because in add_node you've allocated space for the next
entry in the list but you haven't initialized that space.



you should add 'memset(current->next, 0, sizeof(struct name_layout))' or
similar, after you allocate the memory, to get it into a known state rather
than assuming malloc will do this for you.

There are some other problems too, but they're logic errors you should be
able to fix yourself.

Incidentally your code has to run down the entire list each time you add a
node, so your program will get slower and slower as you add more nodes. One
solution is to keep a separate pointer to the tail end of the list, another
is to use a circular list (which in my opinion is neatest method, if you can
afford one extra pointer per structure.) See an example below.

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


typedef struct _item {
struct _item* prev;
struct _item* next;
char* text;
} item;


char* strdupe(char* str)
{
char* buffer = malloc(strlen(str) + 1);
strcpy(buffer, str);
return buffer;
}


item* newList()
{
item* list = malloc(sizeof(*list));
list->prev = list->next = list;
list->text = 0;
return list;
}


item* appendNode(item* list, char* text)
{
item* node = malloc(sizeof(*node));
node->next = list;
node->prev = list->prev;
node->next->prev = node;
node->prev->next = node;
node->text = strdupe(text);
return node;
}


void deleteNode(item* node)
{
node->prev->next = node->next;
node->next->prev = node->prev;
free(node->text);
free(node);
}


void deleteList(item* list)
{
while (list->next != list) {
deleteNode(list->next);
}
free(list);
}


void printList(item* list)
{
item* scan = list;
while ((scan = scan->next) != list) {
printf("%s\n", scan->text);
}
}


int main(int argc, char* argv[])
{
item* list = newList();

appendNode(list, "Peter");
appendNode(list, "Paul");
appendNode(list, "Patrick");

printList(list);

deleteList(list);
return 0;
}

<<<

--
Roger

Roger Willcocks wrote:

memsetting it to 0 will set it to all-bits-zero, which is not /guaranteed/
to set a pointer to NULL (which is what is required here). On the OP's
platform, it will probably work, but that's not the same thing as being
right. He should set the fields separately. (See my parallel reply.)

<snip>

--
Richard Heathfield : binary@eton.powernet.co.uk
"Usenet is a strange place." - Dennis M Ritchie, 29 July 1999.
C FAQ: http://www.eskimo.com/~scs/C-faq/top.html
K&R answers, C books, etc: http://users.powernet.co.uk/eton

Richard wrote:
) memsetting it to 0 will set it to all-bits-zero, which is not /guaranteed/
) to set a pointer to NULL (which is what is required here). On the OP's
) platform, it will probably work, but that's not the same thing as being
) right. He should set the fields separately. (See my parallel reply.)

Maybe you can answer a question that's been bugging me:
If I read the standard correctly, you can set a pointer to NULL by
assigning 0 to it. (I.E. 'char *p = 0'), but I'm nut quite sure.
Did I read the standard correctly ?


SaSW, Willem
--
Disclaimer: I am in no way responsible for any of the statements
made in the above text. For all I know I might be
drugged or something..
No I'm not paranoid. You all think I'm paranoid, don't you !
#EOT

In article <slrnc2vkja.iqo.willem@toad.stack.nl>,
Willem <willem@stack.nl> wrote:
Yes. But this is treated as special. It does not mean
all-bits-zero.

Willem wrote:

Yes, you did. In fact, your compiler may even define NULL as 0.

If 0 isn't a sensible address for a null pointer to point to on a particular
platform, the compiler is required to wave a magic wand to make the code
work anyway. It is /not/ required to wave its magic wand for this code,
though:

memset(&p, 0, sizeof p);

This could, on some platforms, initialise p to a non-NULL value. (Examples
include the Prime 50 series, some Honeywell Bull mainframes, and the CDC
Cyber 180.)

--
Richard Heathfield : binary@eton.powernet.co.uk
"Usenet is a strange place." - Dennis M Ritchie, 29 July 1999.
C FAQ: http://www.eskimo.com/~scs/C-faq/top.html
K&R answers, C books, etc: http://users.powernet.co.uk/eton

On Sun, 15 Feb 2004, Willem wrote:
Extra subtle information which I'm surprised to see Richard did not
provide: the source-code character "0" is what's special, not the
run-time integer constant 0. That is, it's the *compiler* that sees
you've written

char *p = 0;

and invisibly replaces that with

char *p = <some magical "null" constant>;

and *not* the runtime language support. Thus, if you write

int i = 0;
char *p = (char *)i; /* i is 0 here, but... */

you've just invoked Undefined Behavior, because the compiler won't
do its magical replacement of 'i' by '<some magical "null" constant>',
and you'll end up simply converting the bits of the integer "zero"
to the bits of a pointer to 'char'. And that won't produce anything
useful.

NULL <==> 0 is a compile-time phenomenon, not a run-time one.

-Arthur

Arthur wrote:
) and *not* the runtime language support. Thus, if you write
)
) int i = 0;
) char *p = (char *)i; /* i is 0 here, but... */
)
) you've just invoked Undefined Behavior, because the compiler won't
) do its magical replacement of 'i' by '<some magical "null" constant>',
) and you'll end up simply converting the bits of the integer "zero"
) to the bits of a pointer to 'char'. And that won't produce anything
) useful.

Odd, I would have thought that the cast to a pointer would transform 0 to
NULL, but then again, I didn't read that part of the standard so I'll
believe you on your word that it doesn't.


SaSW, Willem
--
Disclaimer: I am in no way responsible for any of the statements
made in the above text. For all I know I might be
drugged or something..
No I'm not paranoid. You all think I'm paranoid, don't you !
#EOT

In article <Pine.LNX.4.58-035.0402151715310.12370@unix41.andrew.cmu.edu>,
Arthur J. O'Dwyer <ajo@nospam.andrew.cmu.edu> wrote:
Minor nit. It's not actually the source code character "0",
but the integer constant zero. So:

char *p = 20 - 4*5;

would also work (at least according to n869, 6.3.2.3/3).

"Richard Heathfield" <dontmail@address.co.uk.invalid> wrote in message <news:c0on35$1j7$1@hercules.btinternet.com>...

For that matter, is there any requirement for integer 0 or floating-point 0
to be all-bits-zero? _The C++ Programming Language, Third Edition_ 5.1.1
says not. Using memset(&p, 0, sizeof(p)) may be /exactly/ the wrong habit
to get into.

--
Joe Foster <mailto:jlfoster%40znet.com> On the cans? <http://www.xenu.net/>
WARNING: I cannot be held responsible for the above They're coming to
because my cats have apparently learned to type. take me away, ha ha!

On Sun, 15 Feb 2004, Joe "Nuke Me Xemu" Foster wrote:
Depends on which integer 0 you're referring to. 'int' 0 is not
required to be all-bits-zero; 'unsigned char' 0, as in the above
code snippet, *is* required to be all-bits-zero. That's why we can
say for sure that 'memset(&foo, 0, sizeof foo)' sets all the bits in
'foo' to zero.

It definitely is the wrong habit to get into, no matter what you're
nitpicking about. 'memset' is supposed to be used for setting bytes
in memory, not for assigning NULL to pointers! For that, you use the
assignment operator '=' in conjunction with the null pointer constant
'NULL'. :-) Any other way is not only superfluous but likely to hide
bugs.

[Note that this thread really belongs on a C programming newsgroup
at this point, such as comp.lang.c or alt.comp.lang.learn.c-c++. If
you want to know more about C's type system, go there.]

[Oh, and yes, in my previous post I did erroneously write "the
source-code character '0'" when what I meant was "the compile-time
integer constant 'zero'" -- the expressions "0" and "(1-1)" are
equivalent null pointer constants when used in a pointer context
at compile time.]

-Arthur

Joe "Nuke Me Xemu" Foster wrote:

<snip>

Yes to ints (but no to floating-point). This int 0 is all-bits-zero was
finally sorted out in a TC to C99.

Yes, it does. This is probably for the same reason that comp.lang.c have
been saying the same thing for years: i.e. the C Standard doesn't guarantee
that int 0 has all-bits-zero representation (and the C++ Standard
"inherits" the C Standard, in finest OOP tradition). Well, they finally
fixed the C Standard, although it might take C++ a while to catch up.

The ISO C Committee felt free to provide the guarantee (at last!) because
nobody anywhere has ever come up with a real world computer in which an int
with all-bits-zero had any value other than 0, so they wouldn't be breaking
anything.


Oh, absolutely. It happens to work for integers, but that's no reason to use
it. :-)

Realistically, you might realistically want to use it on an array of
integers:

memset(arr, 0, sizeof arr);

if you need to "zeroise" the array at, say, the beginning of a loop.

--
Richard Heathfield : binary@eton.powernet.co.uk
"Usenet is a strange place." - Dennis M Ritchie, 29 July 1999.
C FAQ: http://www.eskimo.com/~scs/C-faq/top.html
K&R answers, C books, etc: http://users.powernet.co.uk/eton

"Richard Heathfield" <dontmail@address.co.uk.invalid> wrote in message
news:c0oiu8$l1n$1@hercules.btinternet.com...
Point taken, but to be pedantic I did actually say 'get [the memory] into a
known state' using memset or similar - not 'initialize it to useful starting
values for the algorithm.' Leaving old values laying about in newly
allocated memory invites a whole slew of interesting, platform-dependent and
hard-to-find bugs which are best avoided.

Think 'principle of least surprise.'

From a debugging point of view it would be best if the allocator could
initialize memory to 'unitialized' - if you see what I mean: 'first program
access must be a write.'

Yep.

--
Roger