0

working through some C problems and am hitting a wall with this. I can think of another way or two around this, but I'd like to understand better why my check is failing.

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

int main(int argc, char *argv[256]){
    //Require one alpha only argument if else exit 1
    if(argc < 2){
        printf("Usage: ./vigenere arg1 [arg2] [arg3]...\n");
        return 1;
    }
    for (int i=1;i<argc;i++){
        if(isalpha(argv[i]) == 1){
            return 1;
        }
        printf("%d\n",i);
    }

    //Prompt the user for some plaintext

    //Rotate plaintext by argument

    //Print Rotated Text

    // exit 0
}

The script is working as expected until the isalpha() line. I'd assume argv's that have non-alpha characters in them would !=0 ergo skipping my return(1). However it seems to fail regardless of what is inserted as an argument.

Any thoughts?

Yu Hao
  • 119,891
  • 44
  • 235
  • 294
Gates
  • 117
  • 2
  • 13
  • 1
    What does the debugger show you when you step through the code? What is the content of `argv[i]` just before the segfault? What does the documentation for isalpha() say it accepts as an argument? What are you actually passing to it? – Ken White Nov 03 '16 at 02:10
  • ./vigenere abc Segmentation fault (core dumped) Working on stepping through the code now but I'm not too familiar with gdb. It could be how I'm inserting the information, if the argv contains more than a single character than it would be passing the entire string to isalpha instead of an individual character. – Gates Nov 03 '16 at 02:14
  • 1
    That's not possible with the code you've posted. That content belongs in `argv[0]`, and your for loop starts at index 1. Try again, and read my comment again. – Ken White Nov 03 '16 at 02:15
  • 1
    You are invoking `isalpha()` on a pointer, you need one more dereference. – Nikolai Fetissov Nov 03 '16 at 02:16
  • 1
    `int main(int argc, char *argv[256]){` there's no point in making `argv` a static allocation. I don't know if it even makes a difference. Just use `int main(int argc, char *argv[]){`. – Schwern Nov 03 '16 at 02:20

1 Answers1

3

Turn on warnings with -Wall.

cc -Wall -g test.c   -o test
test.c:13:20: warning: incompatible pointer to integer conversion passing 'char *' to parameter of
      type 'int' [-Wint-conversion]
        if(isalpha(argv[i]) == 1){
                   ^~~~~~~
/usr/include/ctype.h:218:13: note: passing argument to parameter '_c' here
isalpha(int _c)
            ^
1 warning generated.

The problem is this loop.

for (int i=1;i<argc;i++){
    if(isalpha(argv[i]) == 1){
        return 1;
    }
    printf("%d\n",i);
}

It looks like you're assuming argv is a string (ie. char *) and argc is the size of that string. So you're looping through all the characters in argv.

But argv is a list of strings (ie. char **). Instead you probably want the user to pass in a string as one argument and iterate through that (ie. argv[1]) checking for non-alpha characters.

if(argc < 2){
    fprintf(stderr, "Usage: %s <string>\n", argv[0]);
    return 1;
}

char *input = argv[1];
for(int i = 0; i < strlen(input); i++) {
    if( isalpha(input[i]) ) {
        return 1;
    }
}

Note that I'm just checking if isalpha is true, not if it's 1. That's because isalpha returns non-zero if the character tests true. There's no guarantee it will be 1.

Schwern
  • 153,029
  • 25
  • 195
  • 336