0

I have a structure:

struct s 
{
    unsigned size;
    char *var;
};

And an array of pointers to structures

for(i=0;i<rows;i++)
{
    for(j=0;j<columns;j++)
    {
        array[i][j] = (struct s*)malloc(sizeof(struct s));
        printf("Give the size to structure nr. %d: \n",i*columns+j+1);
        scanf("%d",&(array[i][j]->size));
    }
}   

and a function which returns a pointer to an array of random characters with a size chosen by user:

char* ChainOfCharacters(int liczba)
{
    int i;
    char *tabc = NULL;
    tabc = (char*)malloc(liczba*sizeof(char));
    if(tabc==NULL) exit(-1);

    else
    {
        char *tab=NULL;
        tab=tabc;
        for(tabc;tabc<tab+liczba;tabc++)
        {
            *tabc=rand()%(122-97)+97;
        }
    return tabc;    
    }
}

How to connect an array of characters which is a result of this function to pointer char *var of the structure s by using an array of pointers to structures? Is there any mistake in my code?

I tried this: array[i][j]->var=ChainOfCharacters(array[i][j]->size); but it seems to be wrong because i have wrong characters while trying to check this by printf. I don't even know how to write the array of characters on the screen by printf. I tried this printf("%c ", *(array[i][j]->var)); but it shows totaly random signs. I will be very thankful for answers for my questions :)

Piotr Witkoś
  • 334
  • 3
  • 10
  • 1
    First: [mcve]. What is this function `lancuch()` for example? Then, there's at least one bad error in your `ChainOfCharacters`, you're returning `tabc`, which points one past the last character you wrote to your memory. Even if you fix this to return `tab` instead, your result isn't a *string* (it doesn't have a `NUL` byte at the end), so you can't just print this with e.g. `printf("%s" ...)`. –  Aug 27 '17 at 12:35
  • Thanks, I edited and changed lancuch() for ChainOfCharacters. I need just a chain of characters, not string so it's good. So it's the second problem to print character after character. I've already changed tabc for tab in return but it didn't helped ;/ – Piotr Witkoś Aug 27 '17 at 12:48
  • Please study the concept of a [mcve] again. Then please make one. – Yunnosch Aug 27 '17 at 12:53
  • 1
    further unrelated comments: **1.** [don't cast the result of malloc](https://stackoverflow.com/questions/605845/do-i-cast-the-result-of-malloc) in C, **2.** `sizeof(char)` is `1` *by definition*, no need to multiply by it, **3.** Initializing a pointer to `NULL` only to immediately assign it a different value is useless, initialize it directly to the desired value, **4.** use some **spaces** to make your code readable, **5.** an `else` after `exit()` is useless, the code can't be reached ... –  Aug 27 '17 at 12:55
  • For finding the error, a *debugger* will be very helpful ... and for basic debugging strategies without that, read [How to debug small programs](https://ericlippert.com/2014/03/05/how-to-debug-small-programs/) –  Aug 27 '17 at 12:56

2 Answers2

2

First, let's straighten out a few issues with the posted code. size_t is the correct type for array indices and sizes, so the .size member of the s structure should have type size_t. This also means that the ChainOfCharacters() function should take an argument of type size_t, and this has ramifications for the format string specifiers in the calls to printf() and scanf().

A user may not enter a number at the input prompt, in which case no assignment would be made; but since the posted code does not check the value returned from scanf() to verify that a correct input was made, the code would continue with an indeterminate value in that .size field, leading to undefined behavior. The code below checks for this, and exits with an error message if the user fails to input a number here, though this input validation could be further improved.

Note that it is better to use the EXIT_FAILURE macro than to use -1, as this is clearer and more portable.

The ChainOfCharacters() function does not return a string, but only a pointer to an array of characters, so the characters will need to be printed one-by-one.

Note that there is no need to cast the result of malloc() in C, and it is better to use identifers rather than explicit types for operands of the sizeof operator: this is less error-prone and easier to maintain when types change.

The loop that assigns characters in the ChainOfCharacters() function is needlessly complex and contains an error, possibly as a result of this complexity; the tabc pointer points to one past the end of the allocated storage when it is returned. This can be resolved by rewriting the loop to use an index instead of pointer arithmetic. It is generally clearer and less error-prone to use array indexing instead of pointer arithmetic when possible. Avoid using magic numbers: rand() % ('z' - 'a') + 'a' is much clearer in intent than rand()%(122-97)+97 (and don't be afraid to use a little whitespace). Further, note that the C Standard makes few restrictions on the character encodings that may be used by an implementation, and in particular this is not required to be ASCII. The letters of the Latin alphabet need not even be encoded in a contiguous sequence, as is the case with EBCDIC (which still exists in the real world). This is unlikely to be a problem here, but know that there are portable ways to handle this issue.

To assign the result from ChainOfCharacters(), simply assign the pointer to the appropriate array[][] field:

array[i][j]->var = ChainOfCharacters(array[i][j]->size);

To print the contents of the .var fields, iterate over the array, and for each struct, loop over the contents of the allocated storage for .var, printing the characters:

/* print characters */
for (size_t i = 0; i < rows; i++)
{
    for (size_t j = 0; j < columns; j++)
    {
        printf("array[%zu][%zu]->val: ", i, j);
        for (size_t k = 0; k < array[i][j]->size; k++) {
            putchar(array[i][j]->var[k]);
        }
        putchar('\n');
    }
}

After all of this, you will need remember to free() the allocated memory. Here is a complete program that implements these changes.

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

struct s 
{
    size_t size;
    char *var;
};

char* ChainOfCharacters(size_t liczba);

int main(void)
{
    size_t rows = 3;
    size_t columns = 3;
    struct s *array[rows][columns];

    for (size_t i = 0; i < rows; i++)
    {
        for (size_t j = 0; j < columns; j++)
        {
            array[i][j] = malloc(sizeof *array[i][j]);
            printf("Give the size to structure nr. %zu: \n",
                   i * columns + j + 1);

            if (scanf("%zu", &(array[i][j]->size)) != 1) {
                fprintf(stderr, "Incorrect input\n");
                exit(EXIT_FAILURE);
            };
            array[i][j]->var = ChainOfCharacters(array[i][j]->size);
        }
    }

    /* print characters */
    for (size_t i = 0; i < rows; i++)
    {
        for (size_t j = 0; j < columns; j++)
        {
            printf("array[%zu][%zu]->val: ", i, j);
            for (size_t k = 0; k < array[i][j]->size; k++) {
                putchar(array[i][j]->var[k]);
            }
            putchar('\n');
        }
    }

    /* free allocated memory */
    for (size_t i = 0; i < rows; i++)
    {
        for (size_t j = 0; j < columns; j++)
        {
            free(array[i][j]->var);
            free(array[i][j]);
        }
    }

    return 0;
}

char* ChainOfCharacters(size_t liczba)
{
    char *tabc = NULL;
    tabc = malloc(sizeof *tabc * liczba);
    if (tabc == NULL) {
        exit(EXIT_FAILURE);
    } else {
        for (size_t i = 0; i < liczba; i++) {
            tabc[i] = rand() % ('z' - 'a') +'a';
        }
        return tabc;
    }
}

Sample interaction:

Give the size to structure nr. 1: 
1  
Give the size to structure nr. 2: 
2
Give the size to structure nr. 3: 
3
Give the size to structure nr. 4: 
9
Give the size to structure nr. 5: 
8
Give the size to structure nr. 6: 
7
Give the size to structure nr. 7: 
4
Give the size to structure nr. 8: 
5
Give the size to structure nr. 9: 
6
array[0][0]->val: i
array[0][1]->val: lc
array[0][2]->val: psk
array[1][0]->val: lryvmcpjn
array[1][1]->val: bpbwllsr
array[1][2]->val: ehfmxrk
array[2][0]->val: ecwi
array[2][1]->val: trsgl
array[2][2]->val: rexvtj

On choosing correct types

As I said at the beginning of the answer, size_t is the correct type for array indices, as it is an unsigned type that is guaranteed to be able to hold any array index value. But, unsigned is also fine, though unsigned int and size_t may not have the same ranges.

A significant problem in the OP code is that the .size field is unsigned, while the scanf() statement that stores input in this field uses the d conversion specifier, which is meant to be used with ints. According to the Standard, mismatched conversion specifiers and arguments lead to undefined behavior, which includes appearing to work in some instances. But you can't rely on undefined behavior doing what you expect. In the posted code,%u should have been used to store an unsigned value in the .size field. Further, the ChainOfCharacters() function was declared to accept an argument of type int, but was called with an unsigned argument (from .size). This may also lead to implementation-defined behavior, since an unsigned value may not be representable in an int.

Another place that this problem could arise is in the loop that prints the characters. For example, consider:

struct s 
{
    unsigned size;
    char *var;
};

/* ... */

int rows = 3;
int columns = 3;
struct s *array[rows][columns];

/* ... */

/* print characters */
for (int i = 0; i < rows; i++)
{
    for (int j = 0; j < columns; j++)
    {
        printf("array[%d][%d]->val: ", i, j);
        for (int k = 0; k < array[i][j]->size; k++) {
            putchar(array[i][j]->var[k]);
        }
        putchar('\n');
    }
}

Here, k is a signed int, while array[i][j]->size is an unsigned int type, so the value of k will be converted to an unsigned value before the comparison is made. This conversion is well-defined, but can lead to surprises if k is negative.

Enabling compiler warnings will help to detect issues like this at compile time. I always use at least gcc -Wall -Wextra (and -Wpedantic too, but you can probably do without this). Enable compiler warnings, and pay attention to them, and fix them.

ad absurdum
  • 19,498
  • 5
  • 37
  • 60
  • nitpicks: `unsigned` instead of `size_t` here isn't necessarily *wrong*, but might needlessly limit the usable size --- "*avoid using magic numbers: ('z' - 'a') +'a' is much clearer in intent than (122-97)+97*" <- yes, and not hardwired to ASCII, but still not portable as not all encodings have the letters contiguous --- finally instead of the `printf()` in the loop you could use `putchar()` **or** get rid of the whole loop by using `printf("%*s", size, ...)`. –  Aug 27 '17 at 13:10
  • 1
    @FelixPalmen-- all good and fair points. It was silly of me to use `printf()` instead of `putchar()`; my weak defense is that I had converted an earlier `printf()` containing other information, and didn't think. Concerning ASCII, I simply avoided the issue, but this is a valid point. I have updated my answer in both of these respects. – ad absurdum Aug 27 '17 at 13:48
  • 1
    @FelixPalmen-- As for `unsigned` vs `size_t`, I agree that `unsigned` is not wrong, but I think that it is bad to default to this for array indices. OP seemed to be causal about using `unsigned` for the `.size` field, while taking input with the `%d` directive, and used the `int` argument `liczba` in the `ChainOfCharacters()` function, so I changed all to `size_t` for correctness and consistency (though correctness may slightly too strong of a term) without too much comment. – ad absurdum Aug 27 '17 at 13:48
  • 1
    Well, thank you so much for help! The reason of my problem was that my pointer pointed one past the last character in array. So changing returned pointer helped and I didn't know how to print that array after connecting to structure, but this piece of code `printf("%c ", array[i][j]->var[k]);` helped enough :) – Piotr Witkoś Aug 27 '17 at 14:34
  • 2
    @PiotrWitkoś-- I am glad my answer helped. A simple change could make your original code "work", but there were some important issues that I brought up in my answer, and I hope that you understood them. It is _very_ important in robust code to check the value returned from `scanf()` to avoid undefined behavior due to using a variable that has not been assigned a value yet. Also, `unsigned` and `signed` integer types were used too casually in the OP code, and this _can_ lead to serious problems; in fact the posted code had undefined behavior. I have added to my answer to discuss this a bit. – ad absurdum Aug 27 '17 at 15:13
  • @DavidBowling I tried to put your advices to my code, but when I am changing `tabc = (char*) malloc(liczba * sizeof(char));` to `tabc = malloc(sizeof *tabc * liczba);` then I have compilation problem: '[Error] invalid conversion from 'void*' to 'char*' [-fpermissive]' Do you know why does it happen? – Piotr Witkoś Aug 28 '17 at 09:45
  • 1
    @PiotrWitkoś-- What compiler are you using? `malloc()` returns a `(void *)` value, which is being assigned to `tabc`, which is a `(char *)`. C allows you to assign a void pointer to a pointer of another object type, implicitly making the conversion. But, C++ does not allow this, and requires the cast. I suspect that you are using a C++ compiler. – ad absurdum Aug 28 '17 at 12:11
  • @DavidBowling You were right I had a C++ project :S thanks! my last question is: I notised that I can't do like that: `tabc = malloc(sizeof( char*) * liczba);` Could you explain why? – Piotr Witkoś Aug 28 '17 at 16:10
  • 1
    @PiotrWitkoś-- You are allocating storage for `char` values, and `tabc` is a pointer to `char`, which will point to the first element of the allocation. The `sizeof` operator takes either an expression or a parenthesized type as an operand. With `sizeof *tabc`, the operand is the expression `*tabc`, with type `char` (which has a size of 1 by definition). With `sizeof (char *)` you have not an expression, but a parenthesized type (type pointer to `char`); the size of a pointer is implementation dependent, but is almost certainly larger than a `char`.... – ad absurdum Aug 28 '17 at 21:11
  • 1
    @PiotrWitkoś-- So, I would expect this to work, but to allocate more memory than expected. If this is not working (perhaps you are still using a C++ compiler), it is probably because of the casting issue we were discussing. It would need to be `tabc = (char *)malloc(sizeof (char) * liczba)`, or simply `tabc = (char *)malloc(liczba)` (since `sizeof (char)` is always 1, using it here is redundant), or `tabc = (char *)malloc(sizeof *tabc * liczba)` (this is good in case the type of `tabc` is changed later; then there is no need to modify the `sizeof` expression). – ad absurdum Aug 28 '17 at 21:17
0

In your ChainOfCharacters() function, you should be returning tab, not tabc. tab is pointing to the beginning of your chain, tabc is pointing to the end of it.

ad absurdum
  • 19,498
  • 5
  • 37
  • 60
savram
  • 500
  • 4
  • 18
  • It would be helpful if whoever downvoted my answer could tell me why. I made the OP's code work by simply making the small change I said, so I really don't see why it was downvoted. – savram Aug 27 '17 at 13:25
  • I didn't cast the DV, but while this answer may solve the immediate problem, the posted code has other issues. Significantly, if compiled with warnings enabled, there will be warnings about an incorrect format specifier (`%d` expects an `int`, not an `unsigned int`), and perhaps a comparison between `signed` and `unsigned` types in the loop to print characters, e.g., with `for (int k = 0; k < array[i][j]->size; k++) {}`. I think that the answer is somewhat incomplete, but not deserving of a DV; yet everyone has their own opinion of which answers deserve UVs and which deserve DVs.... – ad absurdum Aug 27 '17 at 14:05
  • savram I think that someone downvoted your answer, because you wrote something that was written on this topic two times in earlier discusions. But im a am thankful for your answer so I upvoted that :D – Piotr Witkoś Aug 27 '17 at 14:38