Re: fix 12099 -- quick review?

[David Smith <dsmith at redhat dot com>]

On 12/06/2010 09:27 AM, Rayson Ho wrote:
> Thanks Frank for the review, here is the C++ iostreams version.
> 
> Rayson

I'm not sure you went far enough.  You use C++ iostreams, but not C++
strings.

> +         if (script_file.good())
> +         {
> +           char buffer[PATH_MAX + 1 + 2]; // max path len + \n + #!
> +
> +           script_file.getline(buffer, sizeof(buffer));

Which might not be enough storage if we've got "#!   {PATH}".  So, why
not use the C++ string version of 'getline', like this:

              string buffer;
              getline(script_file, buffer);

> +           if (buffer[0] != '\0')
> +           {
> +             size_t plen = strlen (buffer);
> +
> +             if (plen > sizeof ("#!") && memcmp (buffer, "#!", sizeof
> ("#!")-1) == 0)

I tend to treat strings as strings, so I would have written the above as:
		if (strcmp(buffer, "#!") == 0)

(or if you were using a C++ string you would use the .compare() function)

> +             {
> +                // remove white spaces at the end of the string
> +                plen--;
> +
> +                while (buffer[plen] == '\n' || buffer[plen] == ' ' ||
> buffer[plen] == '\t')
> +                {
> +                  buffer[plen] = '\0';
> +                  plen--;
> +                }

If you were using a C++ string, you could replace the above with

    		   // remove white spaces at the end of the string
                   size_t p2 = buffer.find_last_not_of(" \t\n");
                   if (string::npos != p2)
                       s.erase(p2+1);

and similar changes through the rest of it.

-- 
David Smith
dsmith@redhat.com
Red Hat
http://www.redhat.com
256.217.0141 (direct)
256.837.0057 (fax)