Re: fix 12099 -- quick review?

[Rayson Ho <raysonlogin at gmail dot com>]

Thanks David for the comment. I originally copied some of the I/O code
from the same file & other parts of systemtap.

I will change the code to use C++ strings, and when the testcase is
ready I will check it in.

Rayson



On Mon, Dec 6, 2010 at 1:09 PM, David Smith <dsmith@redhat.com> wrote:
> 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)
>