CODEWITHSUNDEEP
cpointers
Arepa Slayer
Arepa Slayer

Reputation: 443

C pointers inconsistency

I'm working on a program for a C class and I reached a point where I don't know what to do. We are implementing a String library type.

I have my header file (MyString.h)

typedef struct {
    char *buffer;
    int length;
    int maxLength;
} String;

String *newString(const char *str);

The file implementing the functions (MyString.c)

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

String *newString(const char *str) {

// Allocate memory for the String
String *newStr = (String*)malloc(sizeof(String));

if (newStr == NULL) {
    printf("ERROR: Out of memory\n");
    return NULL;
}   

// Count the number of characters
int count;
for (count = 0; *(str + count) != '\0'; count++);
count++;

// Allocate memory for the buffer
newStr->buffer = (char*)malloc(count * sizeof(char));

if (newStr->buffer == NULL) {
    printf("ERROR: Out of memory\n");
    return NULL;
}

// Copy into the buffer
while (*str != '\0')
    *(newStr->buffer++) = *(str++);
*(++newStr->buffer) = '\0';

// Set the length and maximum length
newStr->length = count;
newStr->maxLength = count;

printf("newStr->buffer: %p\n",newStr->buffer); // For testing purposes

return newStr;
}

And a tester (main.c)

#include <stdio.h>
#include "MyString.h"

main() {
char str[] = "Test character array";

String *testString = newString(str);

printf("testString->buffer: %p\n",testString->buffer); // Testing only
}

The problem is that, even though testString is pointing to the String created in newString(), their buffers point to different memory addresses. Why is that?

Thanks in advance

Upvotes: 2

Views: 218

Answers (5)

Patrick Schl&#252;ter
Patrick Schl&#252;ter

Reputation: 11871

As the other colleagues already pointed out, you modified your allocation pointer, which is a no no. Here your example but translated to a more "professional" way.

I would change your structure to:

typedef struct {
   char *buffer;
   size_t length;        /* strings and allocation in C are of type size_t not int */
   size_t alloclength;
} String;

String *newString(const char *str);

And the function would be changed to.

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

String *newString(const char *str)
{
  // Allocate memory for the String
  String *newStr = malloc(sizeof (String));  /* No typecast of void * in C, it's bad taste. */

  if(!newStr) {
    fprintf(stderr, "ERROR: Out of memory\n");     /* Errors are meant to be printed on stderr, not stdio */
    return NULL;
  }   
  // Count the number of characters
  newStr->length = strlen(str);          /* Learn to use the stdlib, there are a lot of usefull functions */
  newStr->alloclength = newStr->length+1;
  // Allocate memory for the buffer
  newStr->buffer = malloc(newStr->alloclength);   /* sizeof (char) is by definition always 1 */
  if(!newStr->buffer) {
    fprintf(stderr, "ERROR: Out of memory\n");
    free(newStr);      
    return NULL;
  }
  // Copy into the buffer
  strcpy(newStr->buffer, str);   /* Because we already scaned the input with strlen, we can use safely the "unsafe" strcpy function. The strcpy will add the trailing 0 */
  printf("newStr->buffer: %p\n",newStr->buffer); // For testing purposes
  return newStr;
}

Upvotes: 3

ChronoTrigger
ChronoTrigger

Reputation: 8617

The question is answered, but I think that there is a piece of code that you should add to avoid a subtle source of memory leak:

// Allocate memory for the buffer
newStr->buffer = (char*)malloc(count * sizeof(char));

if (newStr->buffer == NULL) {
    printf("ERROR: Out of memory\n");
    free(newStr); // free the memory allocated for newStr
    return NULL;
}

Upvotes: 2

Saul
Saul

Reputation: 1002

By using *(++newStr->buffer) and *(newStr->buffer++), you're moving newStr->buffer to essentially point to the end of the string.. You need to modify your code as such:

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

String *newString(const char *str) {
    // Allocate memory for the String
    String *newStr = (String*)malloc(sizeof(String));

    if (newStr == NULL) {
        printf("ERROR: Out of memory\n");
        return NULL;
    }

    // Count the number of characters
    int count;
    for (count = 0; *(str + count) != '\0'; count++);
    count++;

    // Allocate memory for the buffer
    newStr->buffer = (char*)malloc(count * sizeof(char));

    if (newStr->buffer == NULL) {
        printf("ERROR: Out of memory\n");
        return NULL;
    }

    char *pBuffer = newStr->buffer; // don't move newStr->buffer, have another pointer for that.

    // Copy into the buffer
    while (*str != '\0')
        *(pBuffer++) = *(str++);
    *pBuffer = '\0';

    // Set the length and maximum length
    newStr->length = count;
    newStr->maxLength = count;

    printf("newStr->buffer: %p\n", newStr->buffer); // For testing purposes

    return newStr;
}

Upvotes: 3

mjhennig
mjhennig

Reputation: 681

The explanation is pretty simple: You are modifying the buffer pointer in the newString() function:

// Copy into the buffer
while (*str != '\0')
    *(newStr->buffer++) = *(str++);
*(++newStr->buffer) = '\0';

You could use a temporary pointer here (like suggested in the other answers), but I'd like to recommend using the standard functions provided within string.h:

// Count the number of characters
int count;
count = strlen(str) + 1;

// Copy into the buffer
memcpy(newString->buffer, str, count)

Upvotes: 1

rgngl
rgngl

Reputation: 5423

You are modifying the buffer pointer inside the newly created String struct.

You should do:

char *newBuffer = newStr->buffer;
// Copy into the buffer
while (*str != '\0')
    *(newBuffer++) = *(str++);
*(++newBuffer) = '\0';

Upvotes: 2

Related Questions