class library: Pfset - do cleanup on nil stream

[eleses]

Purpose and Motivation

Fixes #4807 Pfset doesn't do cleanup if its sub-pattern yields nil immediately.

Types of changes

• Bug fix

To-do list

• Code is tested
• All tests are passing
• Updated documentation
• This PR is ready for review

[jamshark70]

The fix looks fine to me.

We make it a general practice now to add a unit test for bugfixes whenever practical. This one is easy: I'd add a test method to TestPattern. I think it will be enough to request one event from an empty Pfset, where the init sets a flag to false and the cleanup sets it to true. Then there are 3 final states:

• true: pass
• false: fail (init ran, cleanup didn't)
• nil: fail (neither init nor cleanup ran)

2 asserts would be enough to distinguish these.

I'm not at the computer now, haven't tried it, but I think that will fly.

[eleses]

I've done something a bit more elaborate in terms of testing

	test_Pfset_nilStream {
		var x = 0, y = 0, oe = (), ie = (), clup = EventStreamCleanup.new;
		oe = Pfset({ x = 1 }, p{}, { y = 2 }).asStream.next(ie);
		this.assert(x == 1, "Pfset on nil stream should still call the initializer function");
		this.assert(y == 2, "Pfset on nil stream should still call the cleanup function");
		this.assert(oe.isNil, "Pfset on nil stream should return nil");
		this.assert(ie.size.even, "Pfset on nil stream should add an even number of items to the input event");
		// ie.size is 2 if Pfset adds 'addToCleanup' and 'removeFromCleanup' in sequence (presently);
		// ie.size could be 0 if this add-remove pair is "optimized out" in the future.
		// The present implementation of EventStreamCleanup.update first does the add(s) then the remove(s),
		// so it "cancels out" matched add-remove pairs in the same event. But let's check that too...
		clup.update(ie);
		this.assert(clup.functions.size == 0, "Pfset on nil stream should have no effect on a cleanup-functions set");
	}

Let me see if can add this to the pull request(s) without screwing up the actual fix...

I've done a push of the test; it seems to be showing up below.

[eleses]

The documentation could probably use some spiffing up to explain the behavior. Presently it just says something rather vague about the inti function

func	
Use environment variable syntax (e.g., ~x = 0) to store values in the internal environment. These values are copied into the event prototype before running the supplied pattern.


No promise to execute anything else in it (meh), or basically to even call it if there's nothing to do in the event, i.e if it's nil. But (amusingly) it did basically promise it would run cleanup on nil ("when the supplied pattern runs out of values"):

cleanupFunc
Optional. A function to evaluate when the pattern is stopped, or when the supplied pattern runs out of values. For example, 

So based on that I'm punting on updating the documentation right now; only the init stuff needs a bit of clarification and it can be considered an unrelated issue to this bugfix...

Aside: I see there's some issue with one of the CI servers (something about lint mismatching clang version). I'm pretty sure I didn't cause that. Hopefully it will resolve itself...

[jamshark70]

Test logic looks OK. A few style suggestions.

[jamshark70]

I think this is ok now.

Oh btw... "I see there's some issue with one of the CI servers (something about lint mismatching clang version). I'm pretty sure I didn't cause that." The class library isn't linted, so no, you didn't cause it :D

[mossheim]

thanks! @jamshark70 would this be a breaking change? can it go into 3.11.x?

[jamshark70]

I don't think it's a breaking change -- I'm pretty sure the expectation was to run the cleanup function no matter how many events were yielded in the interim (including 0).

Also I don't recall this class being frequently mentioned -- I think it isn't used that much.