I'm trying to get the hang of using pointers for strings instead of arrays. I'm still finding it a little confusing. Just wondering if I could get some feedback - does this look OK? It works so far, but ....
Code:int length; char *word; void addWord(const char *newWord){ length = strlen(newWord) +1; word = NULL; word = (char *) malloc(length); if (word != NULL) { strlcpy(word, newWord, length); //strlcpy adds '\0' } else { printf("\nCannot find space for word"); exit(0); } }
Looks good, but I would not make such an implementation. Most implementations allows the caller to pass in a buffer and size, so that you do not need to allocate memory via malloc, thus freeing the responsibility to call free.
And calling exit in functions is taboo. You should return an error instead and the caller should handle it.
Originally Posted by Adak
Looks alright and if you really want to get into pointers you might want to write your own routine to copy *newWord to *word instead of using strlcpy() to do the work for you.
Anyways just my .02
You really don't need:
since malloc sets it.Code:word = NULL;
Mainframe assembler programmer by trade. C coder when I can.
Thank you for the replies, very helpful as always
Elysia - I'm not sure exactly what you mean. Is it that it is better to use the stack rather than heap? The direction I'm wanting to go here is eventually to implement a dictionary struct (or possiby class) which contains an array of simpler word structs. Previously I used arrays for each word, but I thought this might be a more 'memory efficient'. I'll have a bit more of a think about this
I also get the idea of providing a buffer and length, but I was trying to make the function do all that work internally. Any chance of a simple example?
Dino - I took out NULL, although somewhere I read something about it being a good thing to initialise pointers to NULL
itCbitC - How does this look?
Code:void ODStrCpy(char *dest, const char *src, int length) { for (int i = 0; i<length; i++) { if (i == length-1) { dest[i] = '\0'; } else if (i< length-1) { dest[i] = src[i]; } } }
Yes, if you do not have an appropriate address to assign to a pointer, then initialising the pointer to be a null pointer is good practice. In this case, you are going to assign it the return value of malloc on the next line, so setting the pointer to NULL is pointless.
Look up a C++ Reference and learn How To Ask Questions The Smart Way
Since this is C, and not C++, the cast is pointless and may hide other problems. Remove it.Code:word = (char *) malloc(length);
--
Mats
Compilers can produce warnings - make the compiler programmers happy: Use them!
Please don't PM me for help - and no, I don't do help over instant messengers.
If it hides problems, then you have a bad compiler. The cast shouldn't hurt.
What I mean is that when you call the function, the code that calls it perhaps have a proper buffer to use, whether that is on the stack or not, that's irrelevant.
So then the caller can pass in that buffer to the function and it uses it as a buffer to store the result. This is the common C way.
The advantages is that the caller can just make a buffer on the stack if it wishes and thus not need to call free later. It decreases complexity. If the caller decides it is necessary, it can also allocate space with malloc and pass it along as the buffer. So you get flexibility.
Thanks for comments laserlight and matsp. NULL and (char*) cast are deleted now.
I've been working away on this and it has evolved a little more. I have a sense that I've made a mistake here, but it is closer to the direction I want to go in.
I have a struct, and an initialisation function "void wordNew". My uncertainty is over whether I should free the temporary pointer (last 2 lines of the function).
In the function I copy the address to the struct's char pointer. My sense from what I have read is that I should ("free what you malloc"), but I am concerned that by freeing the memory , it will get overwritten.
main.c
word.hCode:int main (int argc, const char * argv[]) { Word w; wordNew(&w, "Hello"); return 0; }
word.cCode:typedef struct { char *word; int length; int isAllocated; }Word; void wordNew(Word *wordStruct, const char * newWord);
Code:void wordNew(Word *wordStruct, const char * newWord) { int isAllocated; char* word; int length; length = strlen(newWord) +1; word = malloc(length); //sizeof(char) = 1 byte if (word != NULL) { isAllocated = 1; strlcpy(word, newWord, length); //strlcpy adds '\0' wordStruct->word = word; wordStruct->length = length; wordStruct->isAllocated = isAllocated; } else { isAllocated = 0; } Are either of these necessary?? free(word); // word = NULL; //is this over-cautious??? }
> If it hides problems, then you have a bad compiler. The cast shouldn't hurt.
The problem it hides is your failure to prototype malloc properly, by including stdlib.h
Unfortunately, C89 doesn't mandate prototypes, so the "implicit int" problem would arise, and the existance of the cast masks it.
C99 and C++ have no such implicit rule, so there it isn't a problem.
Though needing to cast malloc in C++ is a different story altogether.
If you dance barefoot on the broken glass of undefined behaviour, you've got to expect the occasional cut.
If at first you don't succeed, try writing your phone number on the exam paper.
You must not free word inside the function, because then it will not be valid outside.
You copy that pointer by assigning it in that struct you pass in.
So it is main's responsibility to free w.word.
I know it does, and it's sad. However, a good compiler will know and warn you about the call of a function without a prototype.
Visual C++ does, and I am sure others should too.
I am just pointing out that. I do not recommend adding casts, but I do not recommend removing them, either.
Last edited by Elysia; 11-06-2008 at 01:23 PM.
Relying on a compiler warning you for doing something wrong is not reliable. It's 10x more likely to warn if you "try to convert integer to pointer" than it is if you have a cast. Some compilers complain about no prototype, others don't. If you are unlucky enough that the code works OK on one system with no warning or failure (perhaps because stdio.h includes stdlib.h), and then you move to another compiler/OS or whatever, the code doesn't work , I promise you that a warning to tell you that "this may not work" is much better.
There is absolutely no benefit in having a cast on malloc. The only time it's needed is if you compile C-code in a C++ compiler.
A point to notice about compilers and warnings: when we "integrate" a project to the main branch, we often get warnings becuase the developers in the project have compiled the code only with the "desktop" compiler, not the embedded compiler. The latter gives warnings for some things that the desktop compiler doesn't warn about (unused variables is a typical case). This stops the "build" from being acceptable to the main branch, so it needs fixing.
--
Mats
Compilers can produce warnings - make the compiler programmers happy: Use them!
Please don't PM me for help - and no, I don't do help over instant messengers.
And even better is compiling in strict C99 mode.
I don't like the idea of non-type strictness. It is alright in languages that has one type to support everything (ie PHP, javascript, etc), but not in languages like C where assigning a non-related type to something else is a bad thing™. That is what casts are for.
Perhaps, for Elysia's sake one could write something like this
Code:#ifdef __cplusplus # define MALLOC_CAST(type, statement) reinterpret_cast< type >( statement ) #else # define MALLOC_CAST(type, statement) statement #endif #define t_malloc (type, size) MALLOC_CAST(type, malloc( size ))
OK, I'm doing whining. I did get my point across.
