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

[Pierre Lebleu]

2017-05-03 3:54 GMT+02:00 Eric Luehrsen <ericluehrsen at hotmail.com>:
>
>
> 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.
> _______________________________________________
> Lede-dev mailing list
> Lede-dev at lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/lede-dev

Ok, I take your remark and comply with it.

-- 
Pierre