[exim-dev] Clang warnings for RC2 #3

Startseite
Nachricht löschen
Nachricht beantworten
Autor: Richard Clayton
Datum:  
To: exim-dev
Betreff: [exim-dev] Clang warnings for RC2 #3
Hands up everyone who understands C operator precedence fully and has De
Morgan's rules at their fingertips.

There's 10 warnings in deliver.c

Six of them are clang getting concerned that the programmer might not get
the result that they wanted, for example

deliver.c:1124:27: warning: '&&' within '||' [-Wlogical-op-parentheses]
       || result == FAIL  && tb->log_fail_output
       ~~ ~~~~~~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~


where the code current says

  1123      if (  tb->log_output
  1124         || result == FAIL  && tb->log_fail_output
  1125         || result == DEFER && tb->log_defer_output
  1126         )


and the intent is

  1123      if (  tb->log_output
  1124         || (result == FAIL  && tb->log_fail_output)
  1125         || (result == DEFER && tb->log_defer_output)
  1126         )


which may or may not be the same thing (because this sort of thing is always
and accident waiting to happen and so I'm always careful to use brackets so
that there's no doubt about it).

There's the same issue around line 2158 and line 2516 and 6287

The second problem is that clang isn't happy about "dangling else"s which
occur at lines 1159, 4152, 7671

1151 if (sender_address[0] != 0 || addr->prop.errors_address)
1152   if (tb->return_output)
1153     {
1154     addr->transport_return = result = FAIL;
1155     if (addr->basic_errno == 0 && !addr->message)
1156       addr->message = US"return message generated";
1157     return_output = TRUE;
1158     }
1159   else
1160     if (tb->return_fail_output && result == FAIL) return_output = TRUE;


clang doesn't take any notice of the indentation so is concerned that the
else at 1159 might be intended to belong to the if at 1151 rather than 1152

easy enough to fix by adding brackets around the inner if

The final warning is at line 4115 where there's an unused return value:

4114    || (  (
4115          !tp->expand_multi_domain || (deliver_set_expansions(next), 1),
4116          exp_bool(addr,
4117              US"transport", next->transport->name, D_transport,
4118              US"multi_domain", next->transport->multi_domain,
4119              next->transport->expand_multi_domain, &md) == OK
4120          )


For those who don't see this often -- the comma operator is being used here
(twice) and clang complains (I think) because the operator precedence means
that it's not necessarily doing what is intended (again my head hurts)

This I believe does the same thing and clang is happy

4114    || (  (
4115          !tp->expand_multi_domain
xxxx          || (deliver_set_expansions(next)
xxxx             ,  /* comma operator */
4116             (exp_bool(addr,
4117                 US"transport", next->transport->name, D_transport,
4118                 US"multi_domain", next->transport->multi_domain,
4119                 next->transport->expand_multi_domain, &md) == OK)
xxxx            )
4120          )


the brackets round the exp_bool() == OK ensure that does what we want and
the comma operator is explicitly called out for clarity. I didn't see any
value in retaining the "1".

I still wouldn't want to audit this code :(

I attach a diff file for all the changes (but there are other ways of
achieving the same thing I'm sure).

-- 
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