2

I am trying to convert the string to upper case, e.g. convert test.pdf to TEST.PDF. However, when I try to print returned value using printf, it prints some junk value. What am I doing wrong?

char *covertToUpper(char *str)
{
    int i = 0;
    int len = 0;

    len = strlen(str);
    char newstr[len+1];

    for(i = 0; str[i]; i++)
    {
       newstr[i] = toupper(str[i]);
    }
    //terminate string
    newstr[i]= '\0';
    return  newstr;
}
1
  • Do you want the contents of str to be changed? If so the code can be made very short.
    – Ed Heal
    Feb 25, 2012 at 4:17

9 Answers 9

11

The reason you are getting junk is because you're allocating"newstr on the stack and then returning its value. This is a big no-no in C. Every function you call afterwards, including the printf() function itself, will trample all over what you just allocated.

C is unfortunately a bit of a dangerous language. It will not stop you from returning a string you allocated on the stack to a calling function even though that memory is no longer safe to use once the function it was declared in returns.

Instead of allocating the string this way, you need to allocate fresh memory on the heap for it using malloc() or calloc() and set newstr to point to it. For example, you could declare:

char newstr = malloc(len);

This will need to be free()d appropriately when it is no longer used, of course.

3
  • genrally mallocing in a conversion is not the best, it ties the conversion function to having to malloc, when there is no need for it Feb 25, 2012 at 4:20
  • The original poster did not explain what API he needed. Perhaps he needed not to alter the original string for whatever reason. In any case, the principle being illustrated here in the answers is that returning a pointer to a stack allocated piece of memory causes trouble, and that's an important principle to know regardless of what the original intent was.
    – Perry
    Feb 25, 2012 at 4:23
  • 1
    @user1204057 Since C uses zero-terminated strings, you must allocate an extra byte for the '\0': malloc(len + 1), or more precisely malloc(sizeof(char)*(len + 1)) Dec 6, 2014 at 12:30
10
#include <stdio.h>
#include <stdlib.h>
#include <string.h>

char *covertToUpper(char *str){
    char *newstr, *p;
    p = newstr = strdup(str);
    while(*p++=toupper(*p));

    return newstr;
}

int main (void){
    char *str = "test.pdf";
    char *upstr;

    printf("%s\n", str);
    upstr=covertToUpper(str);
    printf("%s\n", upstr);
    free(upstr);
    return 0;
}
3
  • This question is looking for an explanation, not simply for working code. Your answer provides no insight for the questioner, and may be deleted. Please edit to explain what causes the observed symptoms. Jun 21, 2017 at 12:38
  • The main point is to use heap instead of using local auto variables.
    – BLUEPIXY
    Jun 21, 2017 at 12:43
  • I see that, but it's probably not evident to the asker without at least some explanatory text. (Actually, I see you using strdup() - you should clarify that that's a common, but not standard, function) Jun 21, 2017 at 12:49
3

you are returning the newstr, but its declared on the stack, so when the function ends, its undefined what will happen. You need to either pass a pointer in to your new str, or malloc one, or just convert the pointer passed in place.

In this case, the most useful thing is to pass in a pointer for the newstre, and a length of the newstr ( saying how much space you are allowed to use in the newstr). This way you aren't tied to a malloc when converting to upper case, you can pass the same pointer for both src and dest and it will do it in place)

If you want then a function that DOES malloc, write a second function ( with a name hinting its allocating memory) which allocs the memory and uses the one that takes the pointers

2

The newstr array doesn't exists outside of the covertToUpper function scope. You must either:

  • dinamically alocate the array (which must be free'd later), changing char newstr[len+1]; to char *newstr = malloc(len + 1)
  • convert the string in place
  • take a destinantion pointer as parameter (and a maxlen for safety too):

    int covertToUpper(const char* src, char* dst, int maxlen) {
      int i, len, max;
      len = strlen(str);
      max = len < maxlen? len : maxlen;
    
      for(i = 0; i < max; ++i) {
        dst[i] = toupper(src[i]);
      }
      dst[i] = '\0';
    
      return i;
    }
    
2

You can transform the string to upper case without using library function. Using some ASCII math can be a better solution:

void toUpper(char *text, char *nText){
    for(int i=0; i<=strlen(text); i++){
        if( (text[i] > 96 ) && (text[i] < 123) ) // if the char is lower case
            nText[i] = text[i] - 'a' + 'A';   //make upper
        else
            nText[i] = text[i]; //do nothing
    }   
}

Function call:

char stuff[] = "test.pdf";
char result[100];

toLower(stuff, result);
printf("toLower: %s", result);
2
  • +1 for caling toLower() when you created a function to make the string uppercase Oct 8, 2014 at 12:32
  • "Using some ASCII math can be a better solution" - only if you somehow know that the target's character coding is ASCII... Jun 21, 2017 at 12:39
1
void strupper(char *s) {
    while (*s) {
        if ((*s >= 'a' ) && (*s <= 'z')) *s -= ('a'-'A');
        s++;
    }
}
2
  • 1
    please elaborate on why, not only on what. Nov 14, 2013 at 22:22
  • 1
    This would almost always work, but maybe you'd be better off by using the islower() and toupper() functions, to handle locale.
    – LSerni
    Nov 14, 2013 at 22:23
1

Other answers have already answered why the OP code wasn't working. Now here is a very simple and (in my opinion) better way to do it:

#include <string.h>
#include <ctype.h>

int strupp(char *s) {
    int i;
    for (i = 0; i < strlen(s); i++)
        s[i] = toupper(s[i]);
    return i;
}

Because usually you want the very same string to be converted to uppercase instead of making a new copy to use more memory. If you really want a new copy, you can always do so before calling the function that converts to uppercase. No need to mix both tasks and make them more complicated.

1

A little correction. There are some other simbols there between 'Z' and 'a': []\^ etc. You can go from 0 to 122 with this restriction

   /*A=63 Z=90 a=97 z=122*/
   while((i>= 0 && i<=64) || (c>=89 && c<=96)){

Have to be ! the oposite.

This is beauty:

 while(str[i])
   {
      putchar (toupper(str[i]));
      i++;
   }
0
#include<stdio.h>
#include<conio.h>
#include<string.h>
void *TakeString(char*);
void main()
{
    char *name;
    clrscr();
    TakeString(name);
    puts(name);
    getch();
}
void *TakeString(char *v)
{
    int i;
    gets(v);
    for(i=0;i<strlen(v);i++)
    {
        v[i]=toupper(v[i]);
    }
}

Your Answer

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

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