[Swan-dev] why there were useless assignments

[D. Hugh Redelmeier]

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.

| 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.