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
