16

I saw the following function in "A Tour of C++", page 12:

int count_x(char const* p, char x)
{
   int count = 0;
   while (p)
   {
      if (*p == x) ++count;
      ++p;
   }
   return count;
}

The line while (p) did not sound right to me. I thought it should've been while (*p). However, not wanting to be too presumptuous, I tested the function with the following code.

int main(int argc, char** argv)
{
   char* p = argv[1];
   char x = argv[2][0];
   int count = count_x(p, x);

   std::cout
      << "Number of occurences of "
      << x << " in " << p << ": " << count << std::endl;
   return 0;
}

When I ran the program, it exited with Segmentation fault (core dumped). I was glad to see that error since the code did not look right to me to start with. However, now am curious. Is the code suggested in the book incorrect or is the compiler not C++11 compliant? The compiler is g++ (GCC) 4.7.3.

What makes the code of count_x strange is that the author, Bjarne Stroustrup, starts with the following implementation before finishing with the one I wrote first.

int count_x(char const* p, char x)
{
   if(p==nullptr) return 0;
   int count = 0;
   for (; p!=nullptr; ++p)
   {
      if (*p == x)
         ++count;
   }
   return count;
}

It made me think twice before concluding this is buggy code. Both versions appear to be buggy.

Chris Drew
  • 14,926
  • 3
  • 34
  • 54
R Sahu
  • 204,454
  • 14
  • 159
  • 270
  • 2
    That code definitely looks buggy to me. Is there any reason you suspect that the compiler isn't C++11-compliant? (Also, the code they're showing off is definitely not good C++ code - it should use `std::string` and the `std::count` algorithm~) – templatetypedef Mar 06 '14 at 22:25
  • @templatetypedef, I will be hard pressed to point a finger at code in a book by the father of C++ and call it buggy without making sure a few times that I am not losing my mind :) :) – R Sahu Mar 06 '14 at 22:31
  • @templatetypedef, the function appears in the first chapter of the book, The Basics. It makes sense that he won't jump into `std::string` and `std::count` yet. – R Sahu Mar 12 '14 at 16:52

3 Answers3

6

This is listed in the Errata for 2nd printing of A Tour of C++:

Chapter 1:

pp 11-12: The code for count_if() is wrong (doesn't do what it claims to), but the points made about the language are correct.

The code in the 2nd Printing now reads:

int count_x(char* p, char x)
    // count the number of occurences of x in p[]
    // p is assumed to point to a zero-terminated array of char (or to nothing)
{
    if (p==nullptr) return 0;
    int count = 0;
    while(*p) {
        if(*p==x)
            ++count;
        ++p;
    }
    return count;
}

There is a similar example in The C++ Programming Language (4th Edition) on which A Tour of C++ was based but it does not have this bug.

Chris Drew
  • 14,926
  • 3
  • 34
  • 54
  • Funny. The code says `count_x`, the text and the errata says `count_if()`. Do you think they will fix this in the next release? – Sassa NF Oct 23 '14 at 09:00
3

gcc has good compatibility at 4.7.3, but you have to compile with -std=c++11. There are charts on the gnu webpage. But yeah, that's not a standard thing, that pointer is just never going to be NULL, at least not until it overflows, so unless you've allocated all the memory above the char *, it's going to segfault. It's just a bug.

user26347
  • 594
  • 3
  • 4
0

This is a corrected version. It checks whether the *p pointer is actually null. It also ensures that there are two input variables.

#include <iostream>

int count_x(char const* p, char x)
{
   int count = 0;
   while (*p) // check if *p is not a null character
   {
      if (*p == x) ++count;
      ++p;
   }
   return count;
}
int main(int argc, char** argv)
{
   if (argc < 3) // check if there are at least two arguments
   {
      std::cout << "Error: Not enough arguments" << std::endl;
      return 1;
   }
   char* p = argv[1];
   char x = argv[2][0];
   int count = count_x(p, x);

   std::cout
      << "Number of occurences of "
      << x << " in " << p << ": " << count << std::endl;
   return 0;
}
tup
  • 139
  • 1
  • 8