1

I am trying to make a program that I think should be simple. I want to ask for a word and store it in a vector with its right size (more \n logically). To do this, the program asks the user to write a word ending with a .. Then, the program reads letter by letter and stores the letters in a string that is created dynamically as the letters are being read.

int main() {
   int i = 0, tam = 0;
   char *cad = (char *)malloc(sizeof(char));
   char c;

   printf("word: ");

   while (c != '.') {
       scanf("%c", c);
       cad[i] = c;
       i++;
       cad = realloc(cad, (i + 1) * sizeof(char));
   }
   cad[i] = '\0';

   for (i = 0; cad[i] == '\0'; i++) {
       tam++;
   }
   printf("tam: %d\n", tam);

   return 0;
}

I have made this but I think that it doesn't do anything useful

11
  • To be pedantic: In C, we don't have vectors. Commented Jul 13, 2020 at 15:22
  • yeah I know @RobertSsupportsMonicaCellio Commented Jul 13, 2020 at 15:23
  • 1
    And you missed "&" in the scanf Commented Jul 13, 2020 at 15:26
  • 1
    and you missed to initialize c too for the first loop Commented Jul 13, 2020 at 15:27
  • 1
    @sonlas10 initialize it with anything except '.' Commented Jul 13, 2020 at 15:29

4 Answers 4

1

It is simpler to use getchar() to read individual bytes from the file. Also check for allocation and reallocation failures:

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

int main() {
    int len = 0;
    char *cad, *new_cad;
    int c;

    printf("word: ");

    cad = malloc(len + 1);
    if (cad == NULL) {
        printf("memory allocation failure\n");
        return 1;
    }
    while ((c = getchar()) != EOF && c != '.') {
        cad[len] = (char)c;
        len++;
        new_cad = realloc(cad, len + 1);
        if (new_cad == NULL) {
            printf("memory allocation failure\n");
            free(cad);
            return 1;
        }
        cad = new_cad;
    }
    cad[len] = '\0';

    printf("tam: %d, string: %s\n", len, cad);
    free(cad);

    return 0;
}

Reallocating the dynamic string one byte at a time is inefficient but implementing a better strategy is an optimisation. First focus on correctness, only optimize if needed as proven by careful benchmarking.

Sign up to request clarification or add additional context in comments.

Comments

0

How to make a dynamic string?

Read carefully a book such as Modern C, then refer to a good C reference website. Read the documentation of your C compiler (perhaps GCC) and debugger (perhaps GDB). Of course, read with attention the documentation of every function (e.g. scanf) that you are using.

 while(c != '.'){
   scanf("%c", c);
   cad[i] = c;
   i++;
   cad = realloc(cad, (i + 1)*sizeof(char));
 }

Your code above is both wrong and inefficient.

It is wrong, since you use scanf (wrongly: should be scanf("%c", &c) but you really want to use getchar) and realloc but don't handle their failure case

It is inefficient, because you probably don't want to reallocate the string at every loop. Unless you are very tight on memory (in practice, improbably) you should consider using realloc only once in a while (since realloc is probably a costly operation). You could for example first malloc a buffer of 128 bytes, and realloc it only when needed (when you did read 127 bytes) to e.g. newsize=3*oldsize/2 (so the second time to 192 bytes, etc...)

And if you compiled with all warnings and debug info (e.g. gcc -Wall -Wextra -g) you'll get useful warnings.

2 Comments

@BasileStarykevitch I guess you also missed the ''&" in scanf
It is even more wrong because c is uninitialized and scanf("%c", c); should be scanf("%c", &c);
0

Made all the changes, this should give you the desired output.

#include <stdio.h>
#include <stdlib.h>
int main(){
   int i = 0, tam = 0;
   char *cad = (char*)malloc(sizeof(char));
   char c = 'l';

   printf("word: ");

   while(c != '.'){
       c= getchar();
       cad[i] = c;
       i++;
       cad = realloc(cad, (i + 1)*sizeof(char));
   }
   cad[i] = '\0';

/*
As pointed out by Bruno, this for loop just calculates the length of the char array, so commented this
for(i = 0; cad[i] == '\0'; i++){
    tam++;
}
printf("tam: %d\n", tam);
*/
printf("tam: %d\n", i);

   return 0;
}

5 Comments

please replace scanf("%c", &c); by a simple getcharand why about to check EOF ? and also proposes to remove the loop about tam because it is useless and the length of the string is already known (warning the code was edited you end loop is wrong)
does it works for you? because it doesn't work for me
@bruno OP wants the no of characters = '\n' right? So, why to remove the last loop
Sorry @bruno just saw it. Will remove it.
c must have type int for proper EOF checking.
0

Two bits of advice:

  1. realloc is a relatively expensive operation, and you really don't want to do it for each character. A common technique is to double the buffer size as necessary - this will minimize the total number of realloc calls. You may wind up with some internal fragmentation, but on average it shouldn't be too bad.

  2. If realloc cannot extend the buffer, it will return NULL and leave the original buffer in place. Thus, you do not want to assign the result of realloc back to the original pointer without checking it first, otherwise you could lose your reference to the previously allocated memory.

It would also be better to use getchar or fgetc for reading the input as opposed to scanf.

So:

#define START_SIZE 2
...
size_t size = START_SIZE;  // tracks the size of the buffer
size_t length = 0;         // tracks the length of the string in the buffer
char *cad = calloc( size, sizeof *cad ); 

/**
 * **ALWAYS** check the result of a malloc, calloc, or realloc call.
 */
if ( !cad )
{
  fprintf( stderr, "Couldn't allocate initial buffer, bailing out here...\n" );
  return EXIT_FAILURE;
}

for ( int c = getchar(); c != '.' && c != EOF; c = getchar() )
{
  /**
   * First, make sure we still have room in our buffer (accounting for the
   * zero terminator) - if we don't, double it.
   */
  if ( length + 1 == size )
  {
    char *tmp = realloc( cad, sizeof *cad * ( 2 * size ) );
    if ( !tmp )
    {
      fprintf( stderr, "realloc failed, exiting loop with what we've read so far\n" );
      break;
    }      
    cad = tmp;
    size *= 2;
  }
  cad[length++] = c;
  cad[length] = 0;   // terminate the string as we go
}

5 Comments

Interesting alternative, but there is a bug: the string is uninitialized if the user types . as the first character... terminating the string as you go is neither needed nor sufficient.
@chqrlie: Fixed initialization by using calloc instead of malloc. Keeping the terminate as you go, so the string will be properly terminated in the event the realloc fails.
While this fixes the problem, it is not entirely consistent as the initial block will be fully initialized to all bits zero, but the reallocated blocks will have uninitialized bytes at the end.
buf[length++] = c; must be cad[length++] = c; and same for buf[length] = 0; // terminate the string as we go which is also better placed after the for rather than inside
a final realloc after the for to reduce the size to the used one can help too, the buffer allocated size if in 2^n which is quickly very long

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Start asking to get answers

Find the answer to your question by asking.

Ask question

Explore related questions

See similar questions with these tags.