2

I have this code:

void changeToCapital(char* str)
{
    int i;
    for (i=0; i<strlen(str); i++) 
    {
        str[i] =str[i] -32;
    }
}

and this method is supposed to get a char* variable, and change it to its uppercase. For some reason I'm getting an error saying EXECUTE_BAD_ACCESS.

The calling function:

char* s = "itzik";
changeToCapital(s);
printf("%s\n",s);

What am I doing wrong here?

7
  • 9
    Are you trying to modify a string literal?
    – user142162
    May 21, 2012 at 17:37
  • Sounds like pointer issues - what's the string pointed to by str when it crashes?
    – wkl
    May 21, 2012 at 17:38
  • im doing str[i], isnt that a char?
    – Itzik984
    May 21, 2012 at 17:38
  • Your function takes a char * though. You need to pass it &str[i]
    – Mike Kwan
    May 21, 2012 at 17:38
  • @MikeKwan nope, the implementation is good (well, it could be good if it was well-implemented...). The issue is that he's modifying a string literal.
    – user529758
    May 21, 2012 at 17:52

2 Answers 2

13

This is most likely because you are passing it a pointer to non-writable memory, such as one obtained from a string literal:

char *ptr = "Hello";
changeToCapital(ptr); // <<== ERROR !

You can change the call to avoid the error:

char ptr[] = "Hello";
changeToCapital(ptr);

On a side note, your change to upper case works only when all letters are in the lower case. You should use toupper(ch) function instead of subtracting 32.

void changeToCapital(char* str) {
    for (; *str = toupper(*str) ; str++)
        ;
}
3
  • and toupper(ch) still won't work for Turkish May 21, 2012 at 17:52
  • @Robert Cooper - Who gives a hoot about Turkish !!
    – DumbCoder
    May 21, 2012 at 17:53
  • 3
    @RobertCooper I think not working for Turkish (and a whole lot of other languages) is still better than not working even for English :) May 21, 2012 at 17:54
4

First:

You are constantly calculating strlen. Instead, you should store the length of the string in a local variable.

Second: you are probably calling the function like this:

char *str = "Hello World";
changeToCapital(str);

This is BAD. "Hello World" is const, and cannot be modified by your program. Instead, you should specify your string as a character array, to ensure that it is immutable:

char str[] = "Hello World";
changeToCapital(str);
4
  • 3
    The value of strlen(str) is not constant in his code. Consider the effect of str[i] =str[i] -32; when str[i] == ' '.
    – Robᵩ
    May 21, 2012 at 17:45
  • @Robᵩ that's just a bad design not to check for unexpected results (and thus handle any special characters as lowercase letters).
    – user529758
    May 21, 2012 at 17:50
  • @H2CO3 - without question OP's program is wrong on many levels. I'm merely observing that Richard's optimization will not produce identical results.
    – Robᵩ
    May 21, 2012 at 17:55
  • Of course, because Richard's fix makes the function work at least for some cases, instead of always causing an error.
    – user529758
    May 21, 2012 at 17:57

Your Answer

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

Not the answer you're looking for? Browse other questions tagged or ask your own question.