compiler: make sure zero-sized array sizeof/alignof/offset of are correct

[skabbes]

Previously discussed here: #2854

[skabbes]

First putting up a test I expect to fail (to confirm test is testing something).

[skabbes]

FYI @deadprogram here is the PR you requested from me.

[deadprogram]

The correction is simple enough. Is this how we want to write the tests for this? @aykevl what do you think?

[aykevl]

The code change looks fine to me. However, the tests seem far more complicated than they need to be. I'd like to keep the tests as simple and straightforward as possible, without unnecessary abstraction. So for example, I would much prefer these tests to be written like this:

v := struct {
    noCompare [0]func()
    data      byte
}{}
println("struct{[0]func(); byte}:", unsafe.Offsetof(v.data) == uintptr(unsafe.Pointer(&v.data)) - uintptr(unsafe.Pointer(&v)))

This is IMHO easier to read, because it only requires local reasoning. You don't need to look at any other code to verify whether it is correct.

Also, generally, I don't think replicating the calculations that are done in compiler/sizes.go in these tests is all that useful. It doesn't test whether these calculations are correct. What is useful is testing whether compiler/sizes.go and LLVM agree, via unsafe.Offsetof and comparing pointer differences for example (as I've done above).

[skabbes]

Changed pushed.

I (obviously) disagree with you regarding what is unnecessary abstraction and the merits of readable code and table-based testing vs non-local reasoning - but reasonable people will disagree with that - it's your codebase so 🤷 .

I do at least think that when there is a bug, there should be a regression test added that exhibits the bug (in this case, sizeof was broken). But again 🤷

Is there a way for me to read up on the philosophy of development in Tinygo? This kind of back and forth is like my least favorite part of contributing to open source, and would love to avoid it if possible.

Please just make whatever edits you want to this PR.

[deadprogram]

Hi @skabbes

My own thought was mainly that we probably do not want to introduce different ways of testing in different parts of the code. Perhaps a proposed change to the testing patterns to be used overall in a separate PR could be looked at. But my interpretation of @aykevl feedback is to try to "write the simplest possible test, and add tests for only the code being changed".

when there is a bug, there should be a regression test added that exhibits the bug

Yes, absolutely. I also think that idea is compatible with the idea of writing a simpler test specific to this change.

Is there a way for me to read up on the philosophy of development in Tinygo?

I think the current contributing page at https://tinygo.org/docs/guides/contributing/ could use some additional philosophy, as it were, to try to explain this part of the process. At the moment it does not contain anything of the sort, and I can see where we can make it a lot easier for people like yourself who are so kind as to wish to help out.

Anyhow, thanks again both for the feedback and code contributions!

[skabbes]

My own thought was mainly that we probably do not want to introduce different ways of testing in different parts of the code.

I would have really appreciated if you mentioned that in your review, along with some specificity. (mention why you thought what I did was "different" or conceptually too complex with respect to the Tinygo codebase). I cannot read your minds.

All I tried to do is take every subtlety and concern along with the actual user facing bug and test combinations of those. I did not see how it could be simpler and still cover those cases, - so I gave up and just copy-pasted the comment in the review (which was accepted and merged!?).

But my frustration is not with testing philosophy, my frustration is trying to read between the lines of the code review to figure out what needs to be changed, and why. It feels like if I don't type verbatim what is in your head, the code will not be accepted - and obviously that is just not very motivating for a programmer.

[deadprogram]

All good points, I'm sure that we maintainers could have done a better job of trying to communicate specifics, which would have been a lot more helpful. We'll work on that.