preprocessor directive not working

Question

I made a program which removes spaces and makes a string upper case by preprocessor directives .its not changing to uppercase

#include <stdio.h>
#include <conio.h>
# define TOUPPER(x) (x-32)
void main(void)
{
    int i,j;
    char str[100],*p;
    clrscr();
    printf("Enter the string:\n");
    gets(str);
    for(i=0;    ;i++)
    {
        if(str[i]=='\0')
        break;
        if(str[i]==' ')
        {
            for(j=i;    ;j++)
            {
             str[j]=str[j+1];
             if(str[j]=='\0')
             break;
            }
         }
         if(str[i]<='a'||str[i]>='z')
         {
            *p=str[i];
            TOUPPER('p');

         }


        }
        puts(str);
    getch();
}

Answer 1

Its because you do

TOUPPER('p');

and it is defined as

(x-32)

This changes 'p' to upper-case but don't save it.

You need to change you define like this

#define TOUPPER(x) ((*x)=((*x)-32))

just do call it like this:

TOUPPER(p);

Answer 2

Your TOUPPER('p') does exactly what it should, nothing. You're subtracting 32 from the numeric value of 'p' and then throwing it away. Note that I'm referring to 'p' the character, not p the pointer.

Answer 3

Perhaps this is what you wanted:

str[i]=TOUPPER(str[i])

(And don't mess around with those pointer p)

Answer 4

Problem is wrong range condition, Macro call with constant argument and you are not assigning the value back.

if(str[i]<='a'||str[i]>='z')
         {
            *p=str[i];
            TOUPPER('p');

         }

Correction:

if(str[i]>='a'||str[i]<='z')
         {
            str[i] = TOUPPER(str[i]);
         }

Answer 5

This isn't what you're asking, but please note you have a serious pointer error in your program:

char str[100],*p;

  ...

if(str[i]<='a'||str[i]>='z')
     {
        *p=str[i];
        TOUPPER('p');

     }

The pointer p is uninitialized (can be pointing to anywhere in memory), and you are assigning the character str[i] to that pointer's memory location. p might be equal to 0x00000000 or 0xCDCDCDCD or something else, but in any case you are writing str[i] to that location without initializing p to an appropriate address.

If you feel you must use pointers, you could do it this way (still doesn't fix the range issue that others mentioned, the <= and >= are backwards):

if (str[i]<='a'||str[i]>='z')
{
   p=&str[i]; // p now points to the address of str[i]
   *p = TOUPPER(*p); // p = the address, *p = the contents of that address
}

Answer 6

Don't believe that using preprocessor macros is always faster than using the functions from <ctype.h>. It is highly probable that these functions are in fact implemented as preprocessor macros themselves. Just write code that is easy to understand, and only "optimize" it if it is necessary.

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

static void
str_toupper(char *str)
{
    char *p;

    for (p = str; *p != '\0'; p++) {
        if (isspace((unsigned char) *p)) {
            memmove(p + 0, p + 1, strlen(p) - 1);
        }
        *p = toupper((unsigned char) *p);
    }
}

int main(void)
{
    char str[100];

    clrscr();
    printf("Enter the string:\n");
    if (fgets(str, sizeof str, stdin) == NULL)
        return 0;

    str_toupper(str);

    printf("%s\n", str);
    getch();
    return 0;
}