[Python-checkins] r59948 - python/trunk/Objects/structseq.c

Mon Jan 14 07:15:01 CET 2008

Neal Norwitz wrote:
> Christian,
>> Can you add some comments to the code? I'll give more detailed code
> review below.

Done
> It would be nice to limit scope for cname, crepr and any others that
> are only used inside the loop.

Does a local variable cost some CPU cycles to free and alloc space on
the stack? Is there a speed difference between
int i;
for(...) {}
and
for (...) [ int i; }
?
> Is 250 enough? How did you calculate? This would be good to doc.
> Also, it seems like it would be better to use either MACRO_SIZE or
> sizeof() calculations so these can't get out of sync.

I chose 250 out of gut feeling. It's more than large enough for os.stat,
time and sys.flags but fits in about 4 lines of output.
> This is a potential buffer overflow. The addition should be checked.
> Also note that strlen() returns size_t which is unsigned and could be
> larger than an int, eg on 64-bit platforms.

I don't see how the code could overflow. It doesn't add the string if
"key=value, " doesn't fit in the buffer before endofbuf.
> If this is the last element the condition above could be false, even
> though its possible for the data to fit.

Oh right, good catch.
> It took me a long time to figure out why 5. I don't like that this is
> dependent on the 5 added to the buffer above and that you are copying
> 3 bytes here. I understood once I saw the -= 2 below. But this
> should be documented and perhaps refactored to try to prevent breakage
> if one part of the code is modified, but not the rest.

You are right, it's too magic and tricky. The code now uses a variable
removelast which should be self explaining.
> If the tuple is empty, this will print TypeNam) rather than
> TypeName(). While this is an unusual use of structseq, it should
> still work.

Fixed
Christian