On Wed, 20 Feb 2019, D. Hugh Redelmeier wrote:
Andrew has introduced several. Here's one:/home/hugh/libreswan/lib/libswan/proposals.c:401:21: warning: Value stored to 'ps' is never read lswlogs(log, as); ps = "-"; as = "+"; ^ ~~~ I like this useless assignment. As I argued below. There are more. Is it still our policy to delete them? I'd like to revert the policy and revert the commits below.
I'd like to not get new (old!) coverity reports I need to justify.... Paul
| From: D. Hugh Redelmeier <[email protected]> | To: Libreswan Development List <[email protected]> | Date: Wed, 5 Dec 2018 18:06:17 -0500 (EST) | | | commit c1f6a2bf6041abb5df918fcfd3fa118bb7c761c7 | | Author: Paul Wouters <[email protected]> | | Date: Tue Dec 4 12:34:57 2018 -0500 | | | | libswan: lswlog_proposal_info() do not set variable sep when no longer used afterwards | | | commit f0b2361d0e6b20e22c7b5fe77dfe8cf1555901bc | | Author: Paul Wouters <[email protected]> | | Date: Tue Dec 4 12:33:29 2018 -0500 | | | | pluto: ikev2_decode_peer_id_and_certs_counted() don't set c when no longer used | | | commit f48425918e69c15edd8d526cbe4b396373d010d3 | | Author: Paul Wouters <[email protected]> | | Date: Tue Dec 4 12:30:51 2018 -0500 | | | | pluto: calc_skeyseed_v2() don't set next_byte when it won't be used anymore | | | commit ce10f9bbaa4f3e0c1926e1a87c75d336b922aa38 | | Author: Paul Wouters <[email protected]> | | Date: Tue Dec 4 11:30:01 2018 -0500 | | | | pluto: initiate_ondemand_body() fix unused setting of loggedit | | In each of these cases, the abstraction / invariant required these | assignments. | | Recommendation: restore these assignments. | | Fallback recommendation: in place of these assignments, put a warning | comment to the effect that the invariant is no longer maintained. | | The code works fine without these assignments. But if you add more | code to one of these routines, the broken abstraction may cause the | code fail. Formally: each assignment was required to maintain an | invariant. | | Here's the tradeoff: | | With assignments | | - lint-like programs complain about a useless assignment (that's why I | previously added an explanatory comment to each of these assignments) | | Without assignment | | - code modified in the obvious way will fail | | | Invariants: | | c1f6a2bf6041abb5df918fcfd3fa118bb7c761c7: | | "sep" indicates what string needs to precede the next chunk | (if any) to be added to the log line. | | c1f6a2bf6041abb5df918fcfd3fa118bb7c761c7 | | next_byte is the index of the next byte of keying material to | use. | | ce10f9bbaa4f3e0c1926e1a87c75d336b922aa38 | | loggedit ("logged it", not "log edit") indicates whether | demandbuf has been logged. | | If an invaraint is not obvious to the reader (I think that it is), | then a comment describing the invariant should be added at the | variable's definition. | From: Paul Wouters <[email protected]> | Date: Thu, 6 Dec 2018 08:43:43 -0500 (EST) | | On Wed, 5 Dec 2018, D. Hugh Redelmeier wrote: | | > In each of these cases, the abstraction / invariant required these | > assignments. | > | > Recommendation: restore these assignments. | | If people add things in a chain without reading through the chain to | realise this, we are lost already? | | > - lint-like programs complain about a useless assignment (that's why I | > previously added an explanatory comment to each of these assignments) | | That didn't prevent coverity from complaining, which was the reason I | removed them. | | Note the loggedit was particularly useless, as it was a chained if else | loop so even extending it would have made the whole thing useless. | | So my preference is to not restore these. _______________________________________________ Swan-dev mailing list [email protected] https://lists.libreswan.org/mailman/listinfo/swan-dev
_______________________________________________ Swan-dev mailing list [email protected] https://lists.libreswan.org/mailman/listinfo/swan-dev
