pgo icon indicating copy to clipboard operation
pgo copied to clipboard

Suggestion to improve readability of the compiled Go code

Open shayanh opened this issue 3 years ago • 4 comments

I suggest changing API of Read and Write methods of ArchetypeInterface.

Current API:

func (iface ArchetypeInterface) Write(handle ArchetypeResourceHandle, indices []tla.TLAValue, value tla.TLAValue) (err error)
func (iface ArchetypeInterface) Read(handle ArchetypeResourceHandle, indices []tla.TLAValue) (value tla.TLAValue, err error) 

Suggested API:

func (iface ArchetypeInterface) Write(handle ArchetypeResourceHandle, value tla.TLAValue, indices ...tla.TLAValue) (err error)
func (iface ArchetypeInterface) Read(handle ArchetypeResourceHandle, indices ...tla.TLAValue) (value tla.TLAValue, err error) 

Motivation: In the most cases, there is no need to use an index when reading or writing; but with the existing API an empty array have to be passed every time.

shayanh avatar Oct 17 '21 06:10 shayanh

I'm leaving this on the backburner for now in favour of paper writing, but I wanted to add some context to the original API decision at least.

It's intended to be read the same way as the MPCal, with handle[indices...] := value.

For the read, it would be easy to use a variadic API instead... for the write, imo it might match my mental model less well... but, well, maybe that's just me being picky.

Would it be still OK to pass nil rather than an empty slice, by the way? Curious, re: alternatives to actually changing the API. I had been thinking for some time that that kind of codegen change would not be hard, I just didn't bother, once I double checked that there is no functional difference between an empty slice (allocated as a singleton, costs basically nothing to summon) and a nil slice.

Anyhow, not opposed to this kind of change, would just want to properly lay out all the alternatives and document the entire reasoning (a simple change as it is...).

fhackett avatar Oct 20 '21 22:10 fhackett

I agree that this is low priority and I just wrote it to not forget it.

It's intended to be read the same way as the MPCal, with handle[indices...] := value.

I didn't understand this.

Would it be still OK to pass nil rather than an empty slice, by the way? Curious, re: alternatives to actually changing the API. I had been thinking for some time that that kind of codegen change would not be hard, I just didn't bother, once I double checked that there is no functional difference between an empty slice (allocated as a singleton, costs basically nothing to summon) and a nil slice.

I also think that using nil should be fine and probably easier to read.

My main motivation for this issue is that currently reading generated Go code is hard, specially the important parts when there is resource read/write. You have to manually filter the empty arrays to find what's going on which is hard a bit in a large program.

shayanh avatar Oct 20 '21 23:10 shayanh

I think you did probably understand... that was all I was proposing, to have the callsites include .Read(handle, nil) and .Write(handle, nil, someVal), rather than []tla.TLAValue{} pasted everywhere.

I defer to your experience reading large-scale generated code, anyhow. Everything else makes sense, and, especially the nil change, I can just add it quickly sometime with minimal breakage (most of the diff would be compiled output files changing).

fhackett avatar Oct 21 '21 00:10 fhackett

I think going with nil is a good choice and also it doesn't break the previous API.

shayanh avatar Oct 21 '21 00:10 shayanh