Create my own strupr function, but it's not working

Question

So I just created my own toupper (to capitalize a single character) and strupr (to capitalize a string). But it doesn't work. So, here is the code.

#include <stdio.h>

int toupper(const int character) {
    if (character >= 97 && character <= 122)
        return (character - 32);
    return character;
}

char *strupr(const char *string) {
    char *result;
    for (int a = 0; a < strlen(string); a++) {
        *(result + a) = toupper(*(string + a));
    }
    return result;
}

int main() {
    char myString[6] = "Hello";
    printf("myString (Before): \"%s\"\n", myString);
    printf("myString (After): \"%s\"\n", strupr(myString));
    return 0;
}

And here is the output:

myString (Before): "Hello"

It just printed out the first line, after that the program stopped. So I need help fixing my code.

Suggestion: Rather than 97, 122, and 32, use `'a'`, `'z'` and `' '`. This is more portable and generally easier to read as now one does not have to memorize the ascii table. If allowed by the assignment, save yourself the hassle and use the library's built in `toupper` function. If not, see about using `isalpha` as not all character encodings, lookin' at you EBCDIC, are nicely organized into contiguous blocks. — user4581301, Dec 04 '18 at 07:04
@user4581301 Not `' '` but `'a' - 'A'`. Although that's not portable to EBCDIC either... — Lundin, Dec 04 '18 at 07:23
What is the value of `result` in `*(result + a) = toupper(*(string + a));`? — chux - Reinstate Monica, Dec 04 '18 at 07:28
@Lundin in an back-handed way that just proved my point. I saw 3 recognizable numbers in a row and misinterpreted the context of the third. [Magic numbers am the bad](https://stackoverflow.com/questions/47882/what-is-a-magic-number-and-why-is-it-bad). — user4581301, Dec 04 '18 at 18:06

snake_style · Accepted Answer · 2018-12-04T07:44:27.917

You are trying to access to uninitialized pointer result. This cause to indefinite behavior:

char * result;
for(int a=0; a<strlen(string); a++)
{
    *result[a] = toupper(string[a]);
}

You need to allocate enough memory, for that array, before accessing:

char toupper(const char character)
{
    if(character >= 'a' && character <= 'z')
        return (character + ('A' - 'a'));
    return character;
}

void strupr(const char *string, char *result)
{
    for(int a = 0; a < strlen(string); a++)
        *(result + a) = toupper(*(string + a));
}


int main()
{
    char myString[6] = "Hello", res[6] = {};
    printf("myString (Before): \"%s\"\n", myString);
    strupr(myString, res);
    printf("myString (After): \"%s\"\n", res);
    return 0;
}

or you can use another variant of strupr, that changes the input string itself:

char* strupr2(char *string)
{
    for(int a = 0; a < strlen(string); a++)
        string[a] = toupper(string[a]);
    return string;
}

int main()
{
    char myString[6] = "Hello", res[6] = {};
    printf("myString (Before): \"%s\"\n", myString);
    printf("myString (After): \"%s\"\n", strupr2(myString));
    return 0;
}

`string[a] = toupper(string[a]);` is UB when `string[a] <0`. More r\bust code uses `string[a] = toupper((unsigned char) string[a]);` — chux - Reinstate Monica, Dec 04 '18 at 07:35
@chux When `string[a] < 0`, then _toupper_ simply returns `string[a]` with no changes. Where is an UB? — snake_style, Dec 04 '18 at 07:45
"toupper simply returns string[a] with no changes." --> perhaps on your machine/compiler, yet C does not define `toupper(int c)` that way. Instead "In all cases the argument is an `int`, the value of which shall be representable as an `unsigned char` or shall equal the value of the macro `EOF`. If the argument has any other value, the behavior is undefined.". So negative values risk _undefined behavior_ (UB). — chux - Reinstate Monica, Dec 04 '18 at 15:07

gsamaras · Answer 2 · 2018-12-04T07:08:25.773

You haven't allocated memory for the result:

char * result;
for(int a=0; a<strlen(string); a++)
{
    *(result+a) = toupper(*(string+a));
}
return result;

As a result, when you try to write to it, you invoke Undefined Behavior.

A simple workaround would be to created a fixed size array in main function for the result, and pass that to your function.

Another approach would be to dynamically allocate memory for the result, like this:

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

int toupper(const int character)
{
    if(character >= 97 && character <= 122)
        return (character - 32);
    return character;
}

char * strupr(const char * string)
{
    char * result = malloc( sizeof(char) * (strlen(string) + 1) );
    if(!result) {
        printf("Malloc failed!\n");
        return "";
    }
    for(unsigned int a=0; a<strlen(string); a++)
    {
        *(result+a) = toupper(*(string+a));
    }
    return result;
}

int main()
{
    char myString[6] = "Hello";
    printf("myString (Before): \"%s\"\n", myString);
    char* res = strupr(myString);
    printf("myString (After): \"%s\"\n", res);
    free(res);
    return 0;
}

If you go with this approach though, please do not forget to free your memory, when you no longer need it (since other answer(s) here forgot to free the memory, causing memory leaks).

Not the cause of the error, but what are the magic numbers in your to upper function? I recommend changing them to the characters themselves (instead of the ASCII codes), like this:

if(character >= 'a' && character <= 'z')
  return (character - ' ');

babon · Answer 3 · 2018-12-04T07:19:47.847

This works:

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

char * strupr(const char * string)
{
    int a;
    char * result = malloc(strlen(string) + 1);
    for(a=0; a<strlen(string); a++)
    {
        *(result+a) = toupper(*(string+a));
    }
    result[a] = '\0';
    return result;
}

int main(void)
{
    char myString[6] = "Hello";
    char *myStringInCaps;
    myStringInCaps = strupr(myString);
    printf("myString (Before): \"%s\"\n", myString);
    printf("myString (After): \"%s\"\n", myStringInCaps);
    free(myStringInCaps);
    return 0;
}

malloc allocates space on the heap. The bits in this space stay put even after the strupr function returns. Also note that \0 is added to the end of the string.

Ideally any *alloced space should be freed, otherwise you risk leaking memory. But, in toy programs such as this, allocated memory is available to be used by the OS after the program exits (but do not ever take this for granted).

Recommend an edit to remind the asker that what is `malloc`ed should also be `free`d. — user4581301, Dec 04 '18 at 07:07
Babon, that could be a nice answer, but please, use `free()`, since now all you do is to create a memory leak! :/ — gsamaras, Dec 04 '18 at 07:09

Create my own strupr function, but it's not working

3 Answers3