For some reason I get problems in my structure, I believe its related to memory allocation For some reason, I get an error when running:
Segmentation fault (core dumped)
I understand that there might be some problem with memory allocation, but I can't detect the problem. I would like to know how to build it correctly and make it more stable. I’ve added comments to th code to make it clear
#include <stdio.h>
#include <stdlib.h>
#include <ctype.h>
/* define the typedef struct node (self-referential structure) */
typedef struct node{
char data;
struct node *next
void print_linked_list(node *head){
unsigned int char_counter = 0;
node *temp = head;
/*iterate the entire linked list and print the data */
while(head->next != NULL){
char_counter;
printf("%c", temp->data);
temp = temp->next;
if (char_counter==0){
******** */
/* this function walk the entire linked-list and free each node */
void free_linked_list(node* head){
while (head != NULL){
node *temp = head;
head = head->next;
free(temp);
}
return;
}
/* ********************************* Main ********************************** */
int main(int argc, char *argv[]){
char ch;
FILE *fog;
/* create the head of the linked list */
node *head = (node*)malloc(sizeof(node));
/* check if the memory allocation succeed */
if (!head){
printf("error 2: memory allocation failed\n");
exit(-2);
}
/* define the file descriptor*/
if (!(fd = fopen(argv[1], "r"))){
printf("error 1: cannot open the selected file.\n");
exit(-1);
}
/* read file char by char until reached EOF */
while ((character = fgetc(fd) != EOF)){
add_node(head, character);
}
print_linked_list(head);
free_linked_list(head);
if (fclose(fd)){
printf("error 3: close error\n");
exit (-3);
}
else {
printf("file closed successfully.\n");
}
return 0;
}
CodePudding user response：
This memory allocation
/* create the head of the linked list */
node *head = (node*)malloc(sizeof(node));
does not make a sense because at least data members of the allocated node are not initialized.
Just write
/* create the head of the linked list */
node *head = NULL;
The function add_node excepts the pointer head declared in main by value. That means that the function deals with a copy of the value of the pointer head. Changing the copy within the function does not reflect on the value of the original pointer head.
Using your approach to the function definition what you need is to change the return value of the function and to return its parameter. For example
/* ************************************************************************ */
/* this function add a node in the end of the linked list */
node * add_node(node *head, char character){
/* check if the head point to NULL (it's an empty linked list),*/
/* otherwise find the last node and add the new node. */
/* create a new node */
node *last_node = NULL;
node *temp = NULL;
if (head == NULL){
//...
}
else{
//...
}
return head;
}
In main you will need to write
head = add_node(head, character);
And there is a bug within the function print_linked_list
while(head->next != NULL){
Change it the following way
void print_linked_list( const node *head){
unsigned int char_counter = 0;
/*iterate the entire linked list and print the data */
for ( const node *temp = head; temp != NULL; temp = temp->next )
{
char_counter;
printf("%c", temp->data);
if (char_counter==0){
printf("\n");
}
}
printf("NULL\n");
}
CodePudding user response：
There are the following issues:
The node allocated for
*headis not initialised. So itsnextmember should not be dereferenced, yet that is what happens in theadd_nodefunction. You should not start with a new node that has no purpose. Instead leave the headNULL, which represents an empty linked list.add_nodereceives the head pointer by value, so it can never hope to change the caller'sheadvariable. One way to solve this, is to make that parameter a pointer to the head pointer, and the caller would pass the address of theirheadpointer.print_linked_listhas a wrongwhilecondition. It should not testhead->next, buttemp, since that is the variable that performs the traversal.To make the output readable,
print_linked_listshould output some delimiter between the node values, so they can be distinguished and do not come out as one string.It would be a good idea to also pass the address of the head pointer to the
free_linked_listfunction, so that it can set the caller's head pointer toNULL.Not a problem, but it is better to avoid code repetition, and only have one line where you call
mallocfor creating a node.In C it is advised to not cast the value returned by
malloc, as that is implied by the variable to which that value is assigned.
Here is the adapted code (without the file handling):
#include <stdio.h>
#include <stdlib.h>
typedef struct node{
char data;
struct node *next;
} node;
// **head: address of head-pointer
void add_node(node **head, char character) {
// One code block for node creation
node *new_node = malloc(sizeof(node));
if (new_node == NULL) {
printf("error 2: memory allocation failed\n");
exit(-2);
}
new_node->data = character;
new_node->next = NULL;
if (*head == NULL) {
*head = new_node;
} else {
node *last_node = *head;
while (last_node->next != NULL) {
last_node = last_node->next;
}
last_node->next = new_node;
}
}
void print_linked_list(node *head) {
unsigned int char_counter = 0;
node *temp = head;
while (temp != NULL) { // corrected condition
char_counter;
printf("%c ", temp->data); // delimit
temp = temp->next;
if (char_counter % 10 == 0) {
printf("\n");
}
}
printf("NULL\n");
}
void free_linked_list(node** head) { // pointer pointer
while (*head != NULL) {
node *temp = *head;
*head = (*head)->next;
free(temp);
}
return;
}
int main(int argc, char *argv[]) {
node *head = NULL; // empty list
add_node(&head, 'a'); // pass address
add_node(&head, 'b');
print_linked_list(head);
free_linked_list(&head); // pass address
return 0;
}