[Swan-dev] why there were useless assignments

[Paul Wouters]

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 <hugh at mimosa.com>
> | To: Libreswan Development List <swan-dev at lists.libreswan.org>
> | Date: Wed, 5 Dec 2018 18:06:17 -0500 (EST)
> |
> | | commit c1f6a2bf6041abb5df918fcfd3fa118bb7c761c7
> | | Author: Paul Wouters <pwouters at redhat.com>
> | | 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 <pwouters at redhat.com>
> | | 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 <pwouters at redhat.com>
> | | 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 <pwouters at redhat.com>
> | | 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 <paul at nohats.ca>
> | 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
> Swan-dev at lists.libreswan.org
> https://lists.libreswan.org/mailman/listinfo/swan-dev
>