Opened 11 years ago

Closed 11 years ago

Last modified 11 years ago

#3919 closed Bug (fixed)

minor code nit in crypto.c

Reported by: Astara Owned by: jordan
Priority: Lowest Milestone: 2.20
Component: libtransmission Version: 2.13
Severity: Trivial Keywords:
Cc:

Description (last modified by jordan)

You might consider replacing the 'for' loop (crypto.c, tr_sha1() ) as in the following:

<   for (;;) {
<     const void *content = (const void *) va_arg (vl, const void *);
<     const int content_len = content ? (int) va_arg (vl, int) : -1;
<     if (content == NULL || content_len < 1)
<       break;
<     SHA1_Update (&sha, content, content_len);
<   }

with a 'while' loop, since that appears to represent the intended code structure:

>   const void * content;
>   const int content_len;
>   while ( content = va_arg (vl, const void *) && 
>         content_len = content ? va_arg(vl, int) : -1) 
>                 SHA1_Update (&sha, content, content_len);

In general, if you are testing 1 or more conditions at the beginning of a loop, use a 'while' loop.

Use a 'for' loop if you need to initialize something before entering the loop or if you have an increment step (or both).

In generally, using the wrong construct of a language impedes understanding of the code and in some cases can produce inferior code.

Second note on the above: in general, it is undesirable to include unnecessary 'casts' (like the ones before 'va_arg' in the 1st part). It can obscure coding errors and is, in this case, unnecessary, since the 2nd arg to 'va_arg' specifies the returned type of the 1st arg.

Just something I noticed...

Change History (7)

comment:1 Changed 11 years ago by jordan

  • Description modified (diff)

comment:2 Changed 11 years ago by jordan

unnecessary casts removed in r11716.

Not sure about the rest of this suggestion -- the patch doesn't compile.

crypto.c: In function ‘tr_sha1’:
crypto.c:50: error: lvalue required as left operand of assignment

comment:3 Changed 11 years ago by jordan

  • Milestone changed from None Set to 2.20
  • Owner set to jordan
  • Status changed from new to assigned

r11717 was simpler than the old version or your patch. ;)

both your patch and r11717 change the semantics of passing in negative values for the length, but it's a private function that never passes negative lengths in, so it doesn't matter...

comment:4 Changed 11 years ago by jordan

  • Resolution set to fixed
  • Status changed from assigned to closed

comment:5 Changed 11 years ago by Astara

I'm too used to ability to mix decl's and code as more modern compilers allow, but not C99, but to diff against your r11716. Not sure my version that didn't check for >0 didn't get copied in -- must have been wrong content in my paste buffer, since my local copy had the check for content>0 :

>   const void * content;
>   int content_len;
44,50c46,48
<   for (;;) {
<     const void *content = va_arg (vl, const void *);
<     const int content_len = content ? va_arg (vl, int) : -1;
<     if (content == NULL || content_len < 1)
<       break;
<     SHA1_Update (&sha, content, content_len);
<   }
---
>   while ( content = va_arg (vl, const void *) && 
>         (content_len = content ? va_arg(vl, int) : -1) > 0 )
>                 SHA1_Update (&sha, content, (const int) content_len);

Of course the new version is 'simpler', it removes the check for passing in -1. But if it isn't necessary, that's better/more optimal. I simply looked at replacing same/w same, not making assumptions about how called functions respond. Ideally, content_len == '0' would be the end case' and forget about special casing a check for <0.for a non-existent content, not "-1", that'd also allow the same shortcut w/no confusion about checking for -1 or not....

i.e. in 'line 50' where you have:

  const int    content_len = content ? va_arg( vl, int ) : -1; 

I'd change the "-1" to a 0, that would prevent calling SHA1_Update on a 0-length content.

comment:6 Changed 11 years ago by Astara

p.s. please don't assume that I carry 'grudges' or attitudes from one issue the next (not saying you necessarily are), but in general, my 'grudge-hold time' tends toward 'low'; i.e. - it's not worth the 'energy'. That doesn't mean I might not be "put off" by some people's attitudes and lose interest in contributing when it is more work to contribute than it is to do the work.

comment:7 Changed 11 years ago by jordan

  • Component changed from Transmission to libtransmission
Note: See TracTickets for help on using tickets.