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

[Christian Heimes]

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