[exim-dev] Clang warnings for RC2 #2

Góra strony
Delete this message
Reply to this message
Autor: Richard Clayton
Data:  
Dla: exim-dev
Temat: [exim-dev] Clang warnings for RC2 #2

Now this clang warning looks like it might actually be a bug :(

cc pdkim.c
pdkim.c:510:36: warning: expression result unused [-Wunused-value]
  *c != isdigit(qp_p[1]) ? qp_p[1] - '0' : toupper(qp_p[1]) - 'A' + 10;
                           ~~~~~~~ ^ ~~~
1 warning generated.


The upstream has

445 /* Check for two hex digits and decode them */
446 if (isxdigit(*qp_p) && isxdigit(qp_p[1])) {
447 /* Do hex conversion */
448 if (isdigit(*qp_p)) {*c = *qp_p - '0';}
449 else {*c = toupper(*qp_p) - 'A' + 10;}
450 *c <<= 4;
451 if (isdigit(qp_p[1])) {*c |= qp_p[1] - '0';}
452 else {*c |= toupper(qp_p[1]) - 'A' + 10;}
453 return qp_p + 2;

whereas Exim has

505 /* Check for two hex digits and decode them */
506 if (isxdigit(*qp_p) && isxdigit(qp_p[1]))
507 {
508 /* Do hex conversion */
509 *c = (isdigit(*qp_p) ? *qp_p - '0' : toupper(*qp_p) - 'A' + 10) << 4;
510 *c != isdigit(qp_p[1]) ? qp_p[1] - '0' : toupper(qp_p[1]) - 'A' + 10;
511 return qp_p + 2;

This doesn't look like a valid refactoring to me -- and clang is on to
something !

So the != needs to be a |=

(though if it was up to me I'd add brackets around the first subtraction for
clarity and write *qp_p as qp_p[0] to make the two expressions look the
same and hence be easier for people to read !

I assume, BTW, that the lack of ORing in the second value breaks something
somewhere, so there's test case(s) missing as well :-(

-- 
richard                                                   Richard Clayton


Those who would give up essential Liberty, to purchase a little temporary
Safety, deserve neither Liberty nor Safety. Benjamin Franklin 11 Nov 1755