since this supposed to be "model code":

• make the spacing of * in types consistent -- generally codebase seems to prefer 1 space to the left, 0 to the right, i.e. how result is declared
• avoid unnecessary abbreviation - receiver instead of rec
• place SetObject(g->sp, result); on a new line

Done. Happy btw when you or anyone spots cosmetic things like this for you to just fix them in the online editor in cases where that would be faster than the discussion/agreement/commit/push cycle. There're enough big things to spend time on. :-)

a couple reasons why i don't do that:

• to be respectful i should make sure it's ok to push to your remote branch first in case you have changes locally. that takes nearly as much time as waiting for you to make the commit yourself.
• some people (myself included) have an amend/rebase-oriented workflow, and adding commits via the web UI would complicate that.
• i believe the incentive should be for contributors to review their own code for style. it saves the most time overall if a reviewer doesn't have to make a new commit or comment in the first place. i realize not everyone may be as attentive or interested in doing that, but it's also no fun if things fall into a pattern where some maintainers are on 'cleanup duty'.
• while it has sometimes resulted in unproductive churn, i think these conversations are actually on the whole quite good at helping to develop and communicate informal community standards and expectations. at least for me, a lot of interesting ideas have come out of that kind of interaction.

I can see all that. But I also worry about the balance between strict and consistent style and the ways in which this can be off-putting to casual contributors, especially if they're more relaxed about such things. As we really do want more people to be resolving issues I think we have to bear that in mind. Sometimes just doing things if they're trivial cosmetic fixes is a way of showing good faith on that and keeping people on board.

For myself, I'll never object to someone pushing something like that. I assume PRs are for (amongst other things) shared work on an issue. If it's sort of git manners thing that's stopping us from doing that, I'd say let's not worry about that. If it's big you can always post a warning and discuss, but these are not big.

the bottleneck lately actually seems to be from a lack of reviews, not a lack of PR submissions.

the bottleneck lately actually seems to be from a lack of reviews, not a lack of PR submissions.

There remain 517 open issues, despite pruning. The issue this PR addresses was open for almost a year, and it's really serious. The oldest issues are going on 6 years old.

i wonder if this should really go in this file.. it is not testing Array so much as gc behavior. but we also don't have a convention yet (under the new unit testing guidelines) for naming files that don't test a class. my first thought would be to place it in a file/class Test_GC, underscore to indicate what follows is not a class name. not sure.

Let's leave it here for now. It's actually a bit of a weird test, as it's really targeting a particular regression. (Could happen to catch other things, but...)

What I think we should really do is make a functionality test for every primitive that does object creation (and maybe just most or all primitives), and then make some specific very targeted tests that check bits of GC functionality, perhaps with test primitives for that purpose. But I think that's more for something like #3384.

Let's leave it here for now.

ok, could you leave a FIXME/TODO so the technical debt is noted then?

It's actually a bit of a weird test, as it's really targeting a particular regression

we have lots of these tests, i like them a lot actually, they're kind of like battle scars :)

What I think we should really do is make a functionality test for every primitive that does object creation (and maybe just most or all primitives), and then make some specific very targeted tests that check bits of GC functionality, perhaps with test primitives for that purpose.

sgtm, some of these tests should be at the C++ level if possible

also if you are going to leave it here i have some code style comments:

• tighten it up by declaring a and b inside the do loop (unless that change eliminates the failure prior to this change? that seems unlikely though) and removing extra empty lines (in general no need to have 2 empty lines in a row)
• 1000.do {|i| to conform to our style guide
• .collect { |lm| lm[0] } to conform to our style guide
• since there is only 1 semantic thing tested here (that gc "works" in a non-regressing way) there should only be 1 assert. details of any failures can be added in the details: parameter of assertEquals. passing an onFailure: argument will cause test running to end immediately on a failure, which is probably not what you want based on the message. i would accumulate details on any failure during the loop (in string format) and then have one assert outside the loop at the end of the test, with the information you're posting in onFailure instead passed as details:.

• tighten it up by declaring a and b inside the do loop (unless that change eliminates the failure prior to this change? that seems unlikely though) and removing extra empty lines (in general no need to have 2 empty lines in a row)

Sorry, but no way. It is not totally clear to me that this won't affect the test, and given how quirky GC issues can be and how particular this is, if we're doing this at all I'd like to be sure we're testing what we thought we were. Providing you can prove conclusively that that won't make a difference, and the outer declaration really bugs you, go ahead, but for me that's a step too far into time wasting finickiness, sorry.

• i would accumulate details on any failure during the loop (in string format) and then have one assert outside the loop at the end of the test, with the information you're posting in onFailure instead passed as details:.

I considered that, but the number of fails isn't relevant, so the end on first failure is actually intentional. The number of fails will vary a bit based on the state of the GC, but tells you nothing of use. Any fail means you've got the problem (or at least a problem).

The rest I'll do shortly.

i'm not trying to waste your time.

there cannot be 1000 asserts made by this test. it will flood test runner output. please change it so only 1 assert is made. and please use pipes instead of arg on lines 175 and 176 as i noted in the earlier review.

i'm not trying to waste your time.

Sorry. Written in haste. I meant it would be a waste of my time, and frankly yours, to go to the trouble of proving that the test was not affected by this, not that your request was intended to waste my time.

please use pipes instead of arg on lines 175 and 176 as i noted in the earlier review.

Ah okay. This is a perfect example of why it would be easier just to make those small changes. You weren't explicit about what you wanted changed, and as I didn't compare it character by character, I noticed the bracket and changed that.

github used to have a "suggest diff" feature that i hope they bring out of beta, because it's exactly what you're looking for. but i will not make commits directly to someone else's branch unless they request or OK that specific diff, personal preference.

there cannot be 1000 asserts made by this test. it will flood test runner output. please change it so only 1 assert is made. and please use pipes instead of arg on lines 175 and 176 as i noted in the earlier review.