Problem passing a constant string into a funciton in C

Started by
6 comments, last by Muzzy A 9 years, 4 months ago

I am trying to create a linked list of contacts in C, and sort them by person length, as an assignment for school. I kinda wanted to use a constructor to simulate object oriented programming. Only problem is, that when i try to enter the name as a string in my constructor function, i get a compile error.

This is how my code looks:


#include "stdafx.h"
#include "stdlib.h"
#include "string.h"

#define MAXL 32

typedef struct post {
    char enamn[MAXL];
    char fnamn[MAXL];
    int langd;
    post *prev;
    post *next;
} POSTTYP;

//API
void sortlist(post *first);

post * newPost(post *first, const char en, const char fn, int ln);

void printlist(post *first);

int _tmain(int argc, _TCHAR* argv[])
{
    post *first;
    newPost(first, "Kalle", "Halvarsson", 185);
    newPost(first, "Johan", "Lovgren", 180);
    printlist(first);
    return 0;
}

post * newPost(post *first, const char *fn, const char *en, int ln) {
    post *p;
    //Reservera minne till posten:
    p = (post*)malloc(sizeof(post));
    
    strcpy(p->fnamn, fn);
    strcpy(p->enamn, en);
    p->langd = ln;
    
    first->prev = p;
    p->next = first;
    *first = *p;
    return p;
}

void removePost(post *first, post* whichpost) {
    post *temp = NULL;

    whichpost->next->prev = whichpost->prev;
    whichpost->prev->next = whichpost->next;
    free(whichpost);

}

void swap(post *a, post *b) {
    post *tnext = NULL;
    post *tprev = NULL;
    tnext = a->next;
    tprev = a->prev;
    a->next = b->next;
    a->prev = b->prev;
    b->next = tnext;
    b->prev = tprev;
}

void sortlist(post *first) {
    post *curr = first;
    while(curr->next != NULL) {
        //This function is incomplete
        curr = curr->next;
    }
}

void printlist(post *first) {
    post *curr = first;
    while (curr != NULL) {
        printf("Förnamn: %s\n",curr->fnamn);
        printf("Efternamn: %s\n",curr->enamn);
        printf("Längd: %s\n",curr->langd);
        curr = curr->next;
    }
}

The error message i get is:

Error 1 : 'newPost' : cannot convert parameter 2 from 'const char [6]' to 'const char'

What am i doing wrong? Overall feedback on the code is very welcome, although note that it is still very unfinished.

Thanks in advance.

Advertisement

post * newPost(post *first, const char en, const char fn, int ln);

/* ... */

post * newPost(post *first, const char *fn, const char *en, int ln) {

Notice the mismatch between your function declaration and definition?

“If I understand the standard right it is legal and safe to do this but the resulting value could be anything.”

Aaahhhhhhhh, so simple, yet so fatal. Thank you SO much, i was pulling my hair over this for hours, haha.

EDIT: My second problem is, "first" is not properly set to "p" in the newPost function. Have i somehow mixed up my pointers? The list works if i instead do this in the main function:


int _tmain(int argc, _TCHAR* argv[])
{
    post *first = NULL;

    first = newPost(first, "Kalle", "Halvarsson", 185);
    first = newPost(first, "Johan", "Lovgren", 180);
    first = newPost(first, "Ingrid", "Brinkenberg", 165);
    first = newPost(first, "Ulrika", "Gillquist", 160);
    printlist(first);
    return 0;
}

It feels a bit ugly though and i would rather if the "first" pointer was set inside the function.

Giving direct answers to posters who are working on homework is discouraged here on gamedev. So, helping you determine problems yourself is the way it's done.

Learning to debug programs can sometimes be a little difficult for new programmers. The primary difficulty is that you may not be sure how to start that process. I'm not sure if you've been taught how to debug, but one method for debugging programs is to "Follow The Data."

That is, if you can't determine a problem by just inspecting your code, provide yourself with some information. N.B., you have to have firmly in your mind: what you want the code to do, and how the code is supposed to do it. As mentioned in that link, print some values to the console as the program runs (you already have a printlist function). If that doesn't lead you to the answer, set a breakpoint in the code, run the program, look at some of the values, and determine if they're correct** at that point in the execution. If they're correct, step through one or more lines and examine values. At some point, you'll find "bad" values. The problem lies in the code after "good" values are found, and where "bad" values are found.

** "correct" means the values are what you expect, knowing what you want the results to be at that point in the execution.

Please don't PM me with questions. Post them in the forums for everyone's benefit, and I can embarrass myself publicly.

You don't forget how to play when you grow old; you grow old when you forget how to play.


My second problem is, "first" is not properly set to "p" in the newPost function. Have i somehow mixed up my pointers?

post *first;

This is a pointer to a post struct. Answer yourself this: where do you actually assign it a value? i.e. where do you say "ok, this is the post struct that you point to".


It feels a bit ugly though and i would rather if the "first" pointer was set inside the function.

In order to modify the value of a variable in a function, you'll need to pass its address into the function.

Thank you all for your replies. To Buckeye, i know perfectly well how to use the debug tool, sadly the debugger in CodeLite only lets you keep track of local variables rather than function arguments (perhaps it can, though i haven't found an option for it). I am not new to programming, although i am new to pointers.

Anyhow, i scrapped the linked list approach since i found that managing the order swapping would become more complicated than it needed to be. I redid everything using a pointer array approach instead, and it is working well save for a few aspects. Specifically, i am struggling with a function where i am to remove a post from the array, and put the last post in the empty space before decrementing the counter. As i am pointing to a pointer array, the levels of indirection become a bit too much for me. Here is the new code, with comments added at the relevant part:


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

#define MAXP 45
#define TRUE 1
#define FALSE 0

typedef int bool;

typedef struct post {
	char fname[32];
	char lname[32];
	int length;
} post;

post * post_new(char *fname, char *lname, int length);
post * post_search(post **list, char *str);
bool post_remove(post **list, int *num, post *p);
bool post_add(post **list, post *p);
void post_sort(post **list, int num);
void post_print(post *p);
void post_printall(post **list, int num);
void post_clearall(post **list, int *num);

void menu_commands();

int main(int argc, char **argv)
{
	post *entries[MAXP];
	post *selected = NULL;
	int num_entries = 0;
	int length;
	bool printlist = FALSE;
	bool printpost = FALSE;
	bool printcommands = TRUE;
	char fname[32];
	char lname[32];
	char input[128];
	
	memset(entries, 0, sizeof(entries));
	
	while(1) {
		system("cls");
		printf("=====================\n");
		printf("=====ADRESS BOOK=====\n");
		printf("=====================\n");
		if (printcommands) {
			menu_commands();
			printcommands = FALSE;
		} else if (printpost && selected != NULL) {
			post_print(selected);
			printpost = FALSE;
		} else if (printlist) {
			post_printall(entries, num_entries);
			printlist = FALSE;
		}
		printf("Number of contacts: %i\n",num_entries);
		printf("What would you like to do?\n");
		fgets(input, 128, stdin);
		
		if (strstr(input, "-new")) {
				printf("Enter first name:\n");
				fgets(fname, 32, stdin);
				printf("Enter last name:\n");
				fgets(lname, 32, stdin);
				printf("Enter length:\n");
				scanf("%i", &length);
				entries[num_entries] = post_new(fname, lname, length);
				num_entries++;
		} else if (strstr(input, "-list")) {
			printlist = TRUE;
		} else if (strstr(input, "-commands")) {
			printcommands = TRUE;
		} else if (strstr(input, "-sort")) {
			post_sort(entries, num_entries);
		} else if (strstr(input, "-clear")) {
			post_clearall(entries, &num_entries);
		} else if (strstr(input, "-remove")) {
			selected = post_search(entries, input);
			if (selected != NULL) {
				post_remove(entries, &num_entries, selected);
			} else {
				printf("No matches found.");
			}
		}
	}
	
	return 0;
}

void menu_commands() {
	printf("Commands:\n");
	printf("'-new' - add contact\n");
	printf("'-remove [name]' - remove best matching contact\n");
	printf("'-clear' - clear the contact list\n\n");
	printf("'-sort' - sort the contact list\n");
	printf("'-list' - show all contacts\n");
	printf("'-commands' - sort command list\n\n");
}

post * post_search(post **list, char *str) {
	int input, i, n=0;
	int index[MAXP];
	char buffer[128];
	
	for (i=0;i<MAXP;i++) {
		if (list[i] != NULL) {
			if (strstr(str, list[i]->fname) != NULL || strstr(str, list[i]->lname) != NULL) {
				index[n] = i;
				n++;
			}
		}
	}
	
	if (n>1) {
		printf("There are multiple matching posts. Did you mean:\n");
		for(i=0;i<n;i++) {
			strcpy(buffer, list[index[i]]->fname);
			strcat(buffer, " ");
			strcat(buffer, list[index[i]]->lname);
			printf("%i. %s",i+1,buffer);
		}
		scanf("%i",&input);
		if (input <= n && input > 0) {
			return list[index[input-1]];
		}
	} else if (n == 1) {
		return list[index[0]];
	}
	return NULL;
}

void post_print(post *p) {
	printf("First name: %s", p->fname);
	printf("Last name: %s", p->lname);
	printf("Length: %i\n", p->length);
}

void post_printall(post **list, int num) {
	int i;
	
	for (i=0;i < num;i++) {
		if (list[i] != NULL) {
			printf("%i. ",i);
			post_print(list[i]);
		}
	}
}

void post_clearall(post **list, int *num) {
	int i;
	for (i=0;i < *num;i++) {
		if (list[i] != NULL) {
			free(list[i]); //Is the freeing of the memory here correct? Or am i just freeing the local pointer?
			list[i] = NULL;
		}
	}
	*num = 0;
}

bool post_remove(post **list, int *num, post *p) {
	int i;
	
	if (p == NULL) return FALSE;
	
	for (i=0;i<MAXP;i++) {
		if (list[i] == p) {
			free(list[i]); 
			list[i] = list[*num]; //This is where it's not working. The swap does not take place in the array, as you'd suspect.
			list[*num] = NULL; //However, i am not sure on what way is correct, it is hard with all the levels of indireciton.
			*num = *num-1; //This line works fine.
			return TRUE;
		}
	}
	return FALSE;
}

bool post_add(post **list, post *p) {
	int i;
	
	for(i=0;i<MAXP;i++) {
		if (list[i] != NULL) {
			list[i] = p;
			return TRUE;
		}
	}
	return FALSE;
}

post * post_new(char *fname, char *lname, int length) {
	post *p = malloc(sizeof(post));
	
	strcpy(p->fname, fname);
	strcpy(p->lname, lname);
	p->length = length;
	
	return p;
}

void post_sort(post **list, int num) {
	post *tmp;
	int i;
	int n = num;
	int newn;
	
	if (list[0] == NULL) return;
	
	while (n>0) {
		newn=0;
		for(i=1;i<n;i++) {
			if (list[i] != NULL) {
				if (list[i-1]->length > list[i]->length) {
					tmp = list[i];
					list[i] = list[i-1];
					list[i-1] = tmp;
					newn = i;
				}
			}
		}
		n = newn;
	}
}

Also, the level of this assignment is not very high, i am just overworking it in order to practice. You don't need to worry about giving direct answers as i have already solved the task itself (which was to sort a list). Thank you for your time.

I would redirect you to your old linkedlist routine and appoint the missusage of pointers, possibly making you see pointers sence of what they realy are (and as they incorporate machine utopia certainly introduced). You have not implemented a linked list in your final solution though

int _tmain(int argc, _TCHAR* argv[])
{
post *first;
newPost(first, "Kalle", "Halvarsson", 185);
newPost(first, "Johan", "Lovgren", 180);
printlist(first);
return 0;
}

post * newPost(post *first, const char *fn, const char *en, int ln) {
post *p;
//Reservera minne till posten:
p = (post*)malloc(sizeof(post));

strcpy
(p->fnamn, fn);
strcpy(p->enamn, en);
p->langd = ln;

first
->prev = p;
p->next = first;
*first = *p;
return p;
}

Realize you pass an unknown unallocated pointer post *first in the main function to the first call of newPost() - and inside this function you perform

first->prev = p; // memory leak, first is an empty pointer does not conatin any member prev you are assigninng to

p->next = first; // assigning a bogus vallue to exisiting correct structre p though
*first = *p; // totaly unneeded, you set values in unexsiting first to values of new entry p .Comment this out

If you instead perform this

int _tmain(int argc, _TCHAR* argv[])
{
post* first=NULL;
// at least make it a magic empty value
newPost(&first, "Kalle", "Halvarsson", 185);
newPost(&first, "Johan", "Lovgren", 180);
printlist(first); // i did not see printlist defintion, do not take this
return 0;
}

and redefine creation of linked list entry of newPost() to this

post * newPost(post** first, const char *fn, const char *en, int ln) {
post *p;
//Reservera minne till posten:
p = (post*)malloc(sizeof(post));

strcpy
(p->fnamn, fn);
strcpy(p->enamn, en);
p->langd = ln;

if (*first==NULL)

{

*first=p; // used to be empty NULL pointer is set to point to first allocated entry p

p->prev=NULL; // you inject new members to be first always, by having prev member set to NULL/zero magic value

p->next=NULL;// p is single first entry with no follower, magic zero next (see the condition we are in?)

}

else

{
p->prev=NULL;
// the new entry p is first (as you intent not be last), acknowledged of it by pointng to previous NULL

p->next=first; // first used to be the first entry existing, it becomes following entry

first->prev=p; // let first entry to know it is being second know, pointing to p as previous one

}
return p;
// return new allocated pointer if so
}

The point of a linked list is not to carry an array of existing members, but instead, carry a single first entry apointage. The allocated objects themslef carry hierarchy they are in by their prev/next members of theirs. If you allocate object, and those objects know next and previous objects, you can restablish/study/recollect their hierarchy any moment, resulting in real interleaved array if so (your final solution). The benefit of linked list is you do not carry array of pointers , but a single pointer - first member, and cointaning objects corretly aware of next and previous. Of course, to keep correct hierarchy you need to alter next and previos members of very object at the point of creating/adding/removing.

You can reform this structure of awareness to whatever you like, for example an interleaved array of structures, which them self exist spread arounf in memory

This could help you to understand pointers, I have even added newPost(post** first for you to be aware of the distinguishing between pointer and pointer to pointer, so a function can manipulate its value (you actualy pass memory that pointer value is at, so function can write to it, so the pointer value changes ,making the pointer point to what function issues)

Maybe too much of load at once, but if you correctly distinguish my post, you will know what pointers are pretty well and what establishing memory them point to is (the pointed object)

Thank you all for your replies. To Buckeye, i know perfectly well how to use the debug tool, sadly the debugger in CodeLite only lets you keep track of local variables rather than function arguments (perhaps it can, though i haven't found an option for it). I am not new to programming, although i am new to pointers.

Buckeye wasn't just talking about the debug tool, he was talking about debugging. The debug tool is a Tool to help you debug, you don't always need it to debug.

EDIT: Said debug too many times, now it has no meaning lol.

This topic is closed to new replies.

Advertisement