3
\$\begingroup\$

I'm currently learning C with "The C Programming Language" by K&R. At the moment I'm at the chapter about pointers (since I come from Java, they are new to me). In the book is an example code for a method getint(), which is supposed to read integers from input, but doesn't handle freestanding '+' and '-' correctly.

As written, getint treats a + or - not followed by a digit as a valid representation of zero. Fix it to push such a character back on the input.

This is what I wrote:

int getint(int *pn)
{
   int c, sign;

   while(isspace(c = getch()))
      ;

   if(c == EOF)
      return EOF;

   if(!isdigit(c) && c != '+' && c != '-')
      return 0;

   sign = (c == '-') ? -1 : 1;

   if(c == '+' || c == '-')
      c = getch();

   if(!isdigit(c)) {
      ungetch(c);
      return 0;
   }

   *pn = 0;
   while(isdigit(c)) {
      *pn = *pn * 10 + c - '0';
      c = getch();
   }

   *pn *= sign;
   ungetch(c);

   return 1;
}

I modified the version inside K&R quite a bit and was wondering if I did a good job at doing so. My getint() is supposed to return EOF when end of file is reached, 0 if it failed to read the current character as an integer and 1 if it successfully wrote the int into *pn.

The K&R code begins with the follwing lines:

int c, sign;
while(isspace(c = getch()))
    ;

if(!isdigit(c) && c != EOF && c != '+' && c != '-') {
    ungetch(c);
    return 0;
}

And I think that the code will stuck if a non-digit, non-'+/-' and non EOF file character is found. Since it'll just unget this char and read it again in the next function call. Is this made on purpose? I left out the ungetch(), since I think that the function is supposed to just read the next char of input in the next function call...

\$\endgroup\$
1
\$\begingroup\$

OP's dropping of ungetc() is incorrect. OP's thought of "it'll just unget this char and read it again in the next function call." assumes the next function call will be a repeated call to getint() - this is not warrented. Since the calling code should inspect the return value and noting it is not 1, execute other code to consume the data in stdin.

if(!isdigit(c) && c != '+' && c != '-') {
  ungetch(c);  // not present in OP code, but present in K&R
  return 0;
}

The though being, if getint(int *pn) cannot read the int, leave stdin alone as much as practicable. Other functionality could be to at least consume 1 character, but I find the first more in keeping with C I/O functions.


A quandary:

In other cases, OP's code below does "Fix it to push such a character back on the input" but IMO does not push back properly. The sign character is not push-backed, only the digit. Instead the sign is consumed by getint().

    c = getch()
...
sign = (c == '-') ? -1 : 1;
if(c == '+' || c == '-')
  c = getch();
if(!isdigit(c)) {
  ungetch(c);  // non-digit push-backed, but not sign.
  return 0;
}

But portably using ungetc() twice in a row is not guaranteed.

One character of pushback is guaranteed. If the ungetc function is called too many times on the same stream without an intervening read or file positioning operation on that stream, the operation may fail. C11dr §7.21.7.10 3

Since code really should "unget" 2 characters (+/- and the following non-digit), but it is not portable - what to do? Since many systems do support pushing back more than 1 character with restrictions, consider attempting it.

Of course, all the consumed white-space characters are not push-backed either. Yet pushing back the sign is prudent - many systems do this with scanf("%d", ...).

Checking the return value of ungetc() would discern success. A slight variation follows:

int c;
int sign = EOF;

while(isspace(c = getch()))
  ;
if(c == EOF)
  return EOF;
sign = c;
if (sign == '+' || sign == '-') 
  c = getch();

if (!isdigit(c)) {
  ungetch(c);  // Pedantic code would check this return value.
  if (sign == '+' || sign == '-') {
    // attempt to push-back sign
    if (ungetch(sign) == EOF) {
      ; // Operation failed
    }
  }
  return 0;
}

...

if (sign == '-') *pn = -(*pn);

Other code issues:

Code does not detect overflow.

Code unnecessarily overflows when scanning INT_MIN. parse_int(char **s) offers a candidate solution.

Code does not differentiate between an EOF received from getch() due to end-of-file and input error. Coding goal is "return EOF when end of file is reached". OTOH, code has no specification concerning input error, so returning EOF for both conditions is reasonable.

\$\endgroup\$
  • \$\begingroup\$ 1. I see the ungetch() makes way more sense, if we handle wrong input in the caller-function. \$\endgroup\$ – LastSecondsToLive Sep 16 '15 at 13:27
  • \$\begingroup\$ 2. This is actually the own K&R version of ungetch() of a previous chapter - I should've mention it. It works with a buffer-array and getch() first empties the array before reading new input with getchar(). So it is possible for me to ungetch() multiple characters. I'm currently just working through K&R and don't have a really good understanding of the standard library. \$\endgroup\$ – LastSecondsToLive Sep 16 '15 at 13:27
  • \$\begingroup\$ 3. Yeah, now I see that the code can easily overflow, but I don't see what happens when scanning INT_MIN. Could you explain a little further please? \$\endgroup\$ – LastSecondsToLive Sep 16 '15 at 13:28
  • \$\begingroup\$ @LastSecondsToLive Example, assume int range is [-128 to 127]. With input like "-128", *pn attempts to get the value of 128 (and overflows) before the final *pn *= sign;. \$\endgroup\$ – chux Sep 16 '15 at 13:28
  • \$\begingroup\$ 4. I didn't even knew that getch() sometimes returns EOF beside when end of file is reached. When does this happen and what can I do to handle this situation? \$\endgroup\$ – LastSecondsToLive Sep 16 '15 at 13:29
0
\$\begingroup\$

Check argument to function before using it, return 0 if NULL

if ( pn != NULL)
{
  return 0;
}

int ungetc(int,FILE*) is the name of the function if you want to use standard C but I understand your compiler has some convenience function. You should also check the return value of ungetch() to see whether it failed or not. It returns EOF when it fails.

Code is for programmers to read so there is absolutely no benefit of making terse expressions by ocular optimization, instead let the compiler optimize and focus on writing clear code.

Use compound statements instead of single line statements for clarity.

if (c == EOF)    
{
  return EOF;
}

You could have added the sign at start of while loop instead, seems more clear somehow but that is just a matter of taste.

*pn = sign;
while (isdigit(c))
{
  *pn = *pn * 10 + c - '0';
  c = getch();
}

The while loop's expression says it should quit when c is not a digit but after that you do not handle the case if c was EOF. Should you then return EOF or 1 ?

Personally I get a bit confused by the function name, when I read getInt() I expect it to return an int instead of getting the value via an argument but that is just me.

\$\endgroup\$
  • \$\begingroup\$ Concerning "Should you then return EOF or 1". Op's code is like scanf("%d", &pn) which returns EOF if EOF occurs and no int or characters are found. It returns 1 if an int is found - even if the following is EOF. \$\endgroup\$ – chux Sep 17 '15 at 15:25

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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