0

I'm trying to overload the >> operator to read a single (created with enum Symbol {e,a,b,c,d};) Symbol: 

istream & operator >> (istream & is, Symbol & sym) {
  Symbol Arr[]={e,a,b,c,d};
  char ch;
  is>>ch;
  if (strchr("eabcd",ch))
    sym=Arr[ch-'e'];
      else {
        is.unget(); 
        is.setstate(ios::failbit);
      }
  return is;
}

But this reads some trash (numbers) instead of what I was looking for, leading to a segmentation fault when trying to print it with my << overload, what am I doing wrong? Edit: Oh, and of course I did add using namespace std; at the start, same with including iostream and cstring.

Gizmo of Arabia
  • 25
  • 8

2 Answers2

1

There's a few things wrong here. First, let's fix your bracing. Just always use braces. It's very difficult to see what's lined up with what:

istream & operator >> (istream & is, Symbol & sym) {
    Symbol Arr[]={e,a,b,c,d};
    char ch;
    is>>ch;
    if (strchr("eabcd",ch)) {
        sym=Arr[ch-'e'];
    }
    else {
        is.unget(); 
        is.setstate(ios::failbit);
    }
    return is;
}

Ok great. Now, what happens if the user inputs something like 'a'. The strchr succeeds, and then you do sym = Arr[ch - 'e']. But ch - 'e' in this case is -4. That's some totally random bit of memory somewhere, so you're getting garbage. To actually use strchr, you'd need to do something like:

const char* options = "eabcd";
if (const char* p = strchr(options, ch)) {
    sym = Arr[p - options];
}

But that's kind of awful. I'd suggest just using a switch:

switch (ch) {
    case 'e': sym = e; break;
    case 'a': sym = a; break;
    ...
    default:
        is.unget();
        is.setstate(ios::failbit);
}

Also, is >> ch could fail and you're not checking that. You should:

istream& operator>>(istream& is, Symbol& sym) {
    char ch;
    if (is >> ch) {
        switch(ch) { ... }
    }
    return is;
}
Barry
  • 286,269
  • 29
  • 621
  • 977
  • But why is it `-4` and not 1? `a` comes after `e` in my code, am I misunderstanding how `strchr` works? – Gizmo of Arabia Mar 18 '16 at 21:56
  • 1
    @GizmoofArabia You're subtracting `char`s. Your enum is irrelevant. – Barry Mar 18 '16 at 21:59
  • @GIzmoofArabia, No. When you're doing substruction, 'a' - 'e' is negative, because you're performing ASCII code substruction. Have a look at [ASCII codes](http://www.asciitable.com/). Look at dec and char columns. – Incomputable Mar 18 '16 at 22:00
  • Thanks for the help, I stuck with `strchr` because I'm not sure if the person checking my work later would be ok with `switch`, `strchr` was kind of suggested for this. So why did you say your 1st solution was "awful", could you elaborate on that? – Gizmo of Arabia Mar 18 '16 at 22:09
  • 1
    @GizmoofArabia Because you're just checking if the inputted char was one of 5 possible values, a `switch` is the more direct way of doing something like that. `strchr` takes a lot longer to reason about here... it's kinda confusing. – Barry Mar 18 '16 at 22:17
0

If ch is 'a', ch - 'e' (97 - 101) will be a negative number (-4), which will lead to accessing the array Arr out of bounds. That leads to undefined behavior.

The way you have your symbols, you'll need to use a switch statement:

switch (ch)
{
   case 'a':
      sym = a;
      break;

   case 'b':
      sym = b;
      break;

   case 'c':
      sym = c;
      break;

   case 'd':
      sym = d;
      break;

   case 'e':
      sym = e;
      break;

   default:
     // Nothing to do
     break;
}

If you want to use Arr, you will need to define Arr as:

 Symbol Arr[]={a,b,c,d,e};

Then, you can access the array as below and avoid the switch statement:

sym=Arr[ch-'a'];  // ch - 'a' is 0 when ch is 'a'
                  // ch - 'a' is 4 when ch is 'e'.
R Sahu
  • 204,454
  • 14
  • 159
  • 270