Skip to content

defer delete formatted strings in the tests in Line Up#187

Merged
rmonnet merged 4 commits intoexercism:mainfrom
0riginaln0:patch-2
Apr 8, 2026
Merged

defer delete formatted strings in the tests in Line Up#187
rmonnet merged 4 commits intoexercism:mainfrom
0riginaln0:patch-2

Conversation

@0riginaln0
Copy link
Copy Markdown
Contributor

@github-actions
Copy link
Copy Markdown

This PR touches files which potentially affect the outcome of the tests of an exercise. This will cause all students' solutions to affected exercises to be re-tested.

If this PR does not affect the result of the test (or, for example, adds an edge case that is not worth rerunning all tests for), please add the following to the merge-commit message which will stops student's tests from re-running. Please copy-paste to avoid typos.

[no important files changed]

For more information, refer to the documentation. If you are unsure whether to add the message or not, please ping @exercism/maintainers-admin in a comment. Thank you!

@rmonnet
Copy link
Copy Markdown
Contributor

rmonnet commented Apr 7, 2026

@0riginaln0, I looked at your change and I am not sure what the best solution is.

There are two main allocators in Odin, the default context.allocator and the temporary allocator, context.temp_allocator. The first one allocate discrete chunks of memory and require the allocated code to be the subject of a delete() to reclaim the memory. The second one uses an arena so there is no way to delete a single chunk, instead you need to clean the entire arena at once using free_all(context.temp_allocator).

There are two ways to format a string: fmt.aprintf() uses context.allocator by default while fmt.tprintf() always use context.temp_allocator. So the exercise function format :: proc(name: string, number: int) -> string, if implemented with aprintf() would require a call to delete() while if implemented with tprintf() would require a call to free_all() (which can be omitted if the program exit right away since the OS would take care of it).

The exercise author uses tprintf() and therefore doesn't need to call delete(). It seems that your PR assumes the student will use aprintf() and therefore correctly call delete(), except that you didn't change the .meta/example.odin so running the test on your PR generates a bunch of warning since you can't free memory in the temporary allocator arena:

[WARN ] --- [2026-04-07 01:31:27] <        0B/        0B> <        0B> (    0/    0) :: line_up.test_format_smallest_two_digit_non_exceptional_ordinal_numeral_10
        +++ bad free        @ 0x118410040 [line_up_test.odin:117:test_format_smallest_two_digit_non_exceptional_ordinal_numeral_10()]

I think both solutions are valid in Odin: 1) use aprintf()/delete() or use tprintf()/free_all(). The problem is we can't really tell which one the student is going to use so we can't always get the test to behave correctly (as far as memory management is concerned).

I am edging towards adding some comments stating that students should not use tprintf() because in a production environment, you would only use the temporary allocator for memory that you control tightly (something that you would allocate and relinquish in your own code, not something you would pass back to a client). There are other exercises which uses tprintf() on the problem side and they would have to be fixed as well. I believe it is ok to use tprintf() in the test itself because this falls in the category where you tightly manage your memory. Until we build an analyzer to provide reviews of the proposed student solutions we will have to assume they comply with the request not to use tprintf().

Does this make sense and do you have any suggestion?

@0riginaln0
Copy link
Copy Markdown
Contributor Author

0riginaln0 commented Apr 7, 2026

@rmonnet

except that you didn't change the .meta/example.odin

yep, sr. I didn't know it's a thing. Mine solution would be basically the same, but using fmt.aprintf.

My suggestion was the same as in the issue #186 . Relying on a temporary allocator just doesn't make sense in some exercises. Or even if they don't rely on temp, they still do produce solutions which have problems with memory management.

Imo until there is an analyzer, it'll be good to guide students by exercise comments and test files warnings.

Copy link
Copy Markdown
Contributor

@rmonnet rmonnet left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I concur with the changes but additional changes are required:

  1. the .meta/example.odin files is causing memory warning now since it is using fmt.tprintf() and, per the discussion of the PR should be using fmt.aprintf(), please modify and check that no memory warnings remains.
  2. A .docs/instructions.appends.md file should be added explaining that the exercise should be solved without using fmt.tprintf() or the temporary allocator for data returned by the exercise API (internal use is still correct as long at the allocation do not bleed to the caller).

Please resubmit with the requested changes.

Thank you for the contribution to the Odin track.

@rmonnet rmonnet added x:action/improve Improve existing functionality/content x:type/content Work on content (e.g. exercises, concepts) labels Apr 7, 2026
Copy link
Copy Markdown
Contributor

@rmonnet rmonnet left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The latest commit looks good. Merging now.

@rmonnet rmonnet merged commit a1024fd into exercism:main Apr 8, 2026
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

x:action/improve Improve existing functionality/content x:type/content Work on content (e.g. exercises, concepts)

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants