libdm: fix -Wparentheses warning.

[jcai19]

This fixes a C operator precedence-related issue exposed by the warning.

Link:
#55

[kergon]

Did you test this? Isn't it the wrong way around? Number of policy arguments plus an extra one if the migration_threshold option will also be included, all times 2.

[m-gupta]

should it be ((seg->migration_threshold != 2048) ? 1 : 0))

[jcai19]

The current implementation is to appease -Wparentheses without changing program semantics, as you pointed out in #56 (comment). :)

[zkabelac]

Yep patch looks patch - it's been nicely covering itself due to test nature

[m-gupta]

Here is the warning log:

lvm2-2.02.187-r3: libdm-deptree.c:2467:81: warning: operator '?:' has lower precedence than '+'; '+' will be evaluated first [-Wparentheses]
lvm2-2.02.187-r3: EMIT_PARAMS(pos, " %u", (seg->policy_argc + (seg->migration_threshold != 2048) ? 1 : 0) * 2);
lvm2-2.02.187-r3: ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ ^
lvm2-2.02.187-r3: libdm-deptree.c:2037:59: note: expanded from macro 'EMIT_PARAMS'
lvm2-2.02.187-r3: if ((w = dm_snprintf(params + p, paramsize - (size_t) p, str)) < 0) {
lvm2-2.02.187-r3: ^~~
lvm2-2.02.187-r3: libdm-deptree.c:2467:81: note: place parentheses around the '+' expression to silence this warning
lvm2-2.02.187-r3: EMIT_PARAMS(pos, " %u", (seg->policy_argc + (seg->migration_threshold != 2048) ? 1 : 0) * 2);
lvm2-2.02.187-r3: ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ ^
lvm2-2.02.187-r3: libdm-deptree.c:2037:59: note: expanded from macro 'EMIT_PARAMS'
lvm2-2.02.187-r3: if ((w = dm_snprintf(params + p, paramsize - (size_t) p, str)) < 0) {
lvm2-2.02.187-r3: ^~~
lvm2-2.02.187-r3: libdm-deptree.c:2467:81: note: place parentheses around the '?:' expression to evaluate it first
lvm2-2.02.187-r3: EMIT_PARAMS(pos, " %u", (seg->policy_argc + (seg->migration_threshold != 2048) ? 1 : 0) * 2);
lvm2-2.02.187-r3: ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~^~~~~~~
lvm2-2.02.187-r3: libdm-deptree.c:2037:59: note: expanded from macro 'EMIT_PARAMS'
lvm2-2.02.187-r3: if ((w = dm_snprintf(params + p, paramsize - (size_t) p, str)) < 0) {
lvm2-2.02.187-r3: ^~~
lvm2-2.02.187-r3: 1 warning generated.

Based on the warning, the existing code was already evaluating "+" before "?" because of C operator precedence.
If ? needs to be evaluated before "+", then this implies that the existing code is buggy. Happy to see clang flagging this.

[m-gupta]

Here is a small snipppet showing the predence of "+" and "?" :

int main() {
int res = 10, cond = 1;
res = res + (cond != 1) ? 5 : 6;
printf("Expected result is 16\n");
printf("Actual %d\n", res);
}

Expected result is 16
Actual 5

So, the current code is buggy if "?" was expected to be evaluated first by the initial author.

[jcai19]

Thanks @m-gupta for the experiments. The reason I make the patch the way it is is indeed to appease the warning without changing the program semantics, since addition has higher precedence over ternary conditional. Changing it based on the comment from #56 (review) would change the program semantics, or is that expected @kergon?

[kergon]

When you get a compiler warning, the idea is not to unthinkingly suppress the warning, but to check what the code was supposed to do, understand it, and then resolve the warning accordingly.

There are two possible patches here. Look at each option in the context of the lines around it then decide which of the two is the one that makes sense.

[jcai19]

Thanks for the comment. I've updated the code accordingly.