0

I'm trying to figure out why this doesn't work, but I'm not really sure. 

void main(void){ 
    char* s = "aBCde F. hjiodhfvev, . efof Gh";
    to_lower_case(s); 
    printf("%s",s); 
}

The function to_lower_case() is just a random function that does what its name suggests. I guess it has something to do with the string being declared as a constant.

Whooper
  • 575
  • 4
  • 20
Nerd
  • 43
  • 4
  • 1
    And where is the code for `to_lower_case`? – sjsam Jan 31 '18 at 20:08
  • 6
    Modifying a string literal is undefined behavior (so, you should never assign a string literal to a `char*`). Instead declare the variable as a char array, which *copies* the string literal: `char s[] = "the string literal";` – Ray Hamel Jan 31 '18 at 20:08
  • Also, `void` main? Are you planning to learn `premature C` then unlearn it to learn latest standards? – sjsam Jan 31 '18 at 20:09
  • There's nothing wrong about a void main. – Nerd Jan 31 '18 at 20:12
  • 2
    @Nerd: See [What should `main()` return in C and C++?](https://stackoverflow.com/questions/204476/) for a complete disquisition on why `void main()` is only faintly acceptable on MS Windows and essentially nowhere else. – Jonathan Leffler Jan 31 '18 at 20:13
  • Note: finding the dupe was easy - I copypasted your exact title into a popular search engine and got: 'About 1,140,000 results', with the dupe as top entry:( – Martin James Jan 31 '18 at 20:17
  • I'll accept it as dupe. Sorry. – Nerd Jan 31 '18 at 20:18

2 Answers2

2

This is the problem

char* s = "aBCde F. hjiodhfvev, . efof Gh";

String literals (the ones in quotes) are read-only. You cannot modify them.

You should declare it as

char s[] = "aBCde F. hjiodhfvev, . efof Gh";
Pablo
  • 13,271
  • 4
  • 39
  • 59
  • Well, are you sure this is the issue? This could be, but everything `to_lower_case()` does is still under the hoods.. – sjsam Jan 31 '18 at 20:12
  • This was the issue, although I should've included the code for the lower case function aswell. – Nerd Jan 31 '18 at 20:14
  • @sjsam yes I'm sure, even without knowing what `to_lower_case()` does. This is a common pitfall for beginners. Non-beginner do not make this mistake. – Pablo Jan 31 '18 at 20:15
1

Correct. Strings provided as string-literals are read-only. You will need to copy the string to mutable memory first:

char* s_readonly = "foobar";
size_t s_length = strlen( s_readonly );
char* s_mutable = malloc( s_length + 1 );
assert( s_mutable );
errno_t err = strcopy_s( s_mutable, s_length, s_readonly );
assert( err == 0 );
to_lower_case( s_mutable ); 
puts( s_mutable );
free( s_mutable );
  • My code performs an explicit copy to the heap. This code could be made simpler by assigning the string literal to a char[n]-type. This has the advantage of allowing static sizeof() to work which is faster O(1) than strlen which is O(n).
  • printf("%s\n", s) can be replaced with puts which is faster as there's no format-string to parse.
  • I use strcopy_s over the insecure strcopy. Always check the returned error code!
  • And always call free after malloc. If you're dealing with short strings you could use alloca instead which is faster and doesn't require the use of free.

Alternatively, just this:

char s_mutable[] = "foobar";
to_lower_case( s_mutable ); 
puts( s_mutable );

The advantage here (besides terseness) is that s_mutable is mutable right away, and it means that sizeof( s_mutable ) == 12.

Dai
  • 141,631
  • 28
  • 261
  • 374