[LEDE-DEV] [PATCH 5/7] firewall3: add UBUS support for ipset sections

[Eric Luehrsen]

On 05/02/2017 05:36 PM, Paul Oranje wrote:
> Assignment within a condition is easily read by (dyslectic) humans as a test for equality (==) and is for that reason als better avoided.
> Paul
>
>> Op 2 mei 2017, om 18:43 heeft Philip Prindeville <philipp_subx at redfish-solutions.com> het volgende geschreven:
>>
>>
>>> On May 2, 2017, at 6:15 AM, Pierre Lebleu <pme.lebleu at gmail.com> wrote:
>>>
>>> Hi Philip,
>>>
>>> 2017-04-29 3:11 GMT+02:00 Philip Prindeville <philipp_subx at redfish-solutions.com>:
>>> Inline…
>>>
>>>
>>> [snip]
>>>> +             if (!(ipset = fw3_alloc_ipset(state)))
>>>
>>>
>>> Minor nit…  Assignments inside of conditionals are a bear to step through in a debugger like GDB.
>>>
>>> -Philip
>>>
>>> It is a trivial assignment and it is already done in this style along the file.
>>>
>>> --
>>> Pierre
>>>
>>
>>
>> It’s not about trivial vs. nontrivial.  It’s about whether you could step through the assignment with (say) gdb, execute just the assignment, examine the value, and then step through the “if”.  And the answer is, “you can’t”.  Because gdb is a source level debugger where the unit of source is the “line”.
>>
>> (Actually, it’s also the unit of source for gcc when it generates debugging symbols.)
>>
>> The way to separate to 2 individual statements in C (for the purposes of gdb debugging) is to put them on separate lines.  Yes, that’s a glaring limitation of gcc and gdb, but that’s our reality.
>>
>> As for what’s already done in this style in the file… Having separate assignments and tests is *also* done, and indeed it’s done more often.
>>
>> -Philip

Others may have been clever but it doesn't mean a construct should 
continue. This if-assignment combo and other clever C constructs are 
frowned upon in popular software quality standards. Many simply are due 
to the above mentioned maintainability issues. Some industries may be 
more sensitive, but the rationale for avoidance is solid.