Conversation
|
Very nice! I wanted to contribute an arena allocator myself but you beat me to it 👍🏾 |
Burnett01
left a comment
Burnett01
left a comment
There was a problem hiding this comment.
Did a code review and after the suggested changes it works:
So please incorporate the suggestions :)
std/arena.zc
Outdated
|
|
||
| fn alloc<T>(self) -> T* { | ||
| let ptr: T* = (T*)self.alloc_bytes(sizeof(T)); | ||
| if (ptr != 0) { |
There was a problem hiding this comment.
Please use NULL because 0 lead to segfault when I tested your code, because it will compare with strcmp.
out.c: In function ‘Arena__alloc’:
out.c:165:6: warning: argument 2 null where non-null expected [-Wnonnull]
165 | if ((strcmp(ptr, 0) != 0)) {
| ^~~~~~
| if (ptr != 0) { | |
| if (ptr != NULL) { |
std/arena.zc
Outdated
| fn alloc_n<T>(self, count: usize) -> T* { | ||
| let size = sizeof(T) * count; | ||
| let ptr: T* = (T*)self.alloc_bytes(size); | ||
| if (ptr != 0) { |
There was a problem hiding this comment.
Please use NULL because 0 leads to segfault because it will compare with strcmp.
| if (ptr != 0) { | |
| if (ptr != NULL) { |
std/arena.zc
Outdated
| fn dup_str(self, src: char*) -> char* { | ||
| let len = strlen(src); | ||
| let ptr: char* = (char*)self.alloc_bytes(len + 1); | ||
| if (ptr != 0) { |
There was a problem hiding this comment.
Please use NULL because 0 leads to segfault because it will compare with strcmp.
| if (ptr != 0) { | |
| if (ptr != NULL) { |
| let len = strlen(src); | ||
| let ptr: char* = (char*)self.alloc_bytes(len + 1); | ||
| if (ptr != 0) { | ||
| strcpy(ptr, src); |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
std/arena.zc
Outdated
| } | ||
|
|
||
| fn free(self) { | ||
| if (self.data != 0) { |
There was a problem hiding this comment.
Please use NULL because 0 leads to segfault because it will compare with strcmp.
| if (self.data != 0) { | |
| if (self.data != NULL) { |
examples/arena_test.zc
Outdated
| let mark = arena.save(); | ||
| printf("Savepoint at: %zu\n", mark); | ||
|
|
||
| let extra: int* = arena.alloc_n<int>(100); |
There was a problem hiding this comment.
This prints a warning because extra is not used. So it's fine to change it to:
| let extra: int* = arena.alloc_n<int>(100); | |
| let _: int* = arena.alloc_n<int>(100); |
Burnett01
left a comment
Burnett01
left a comment
There was a problem hiding this comment.
Please add the following file in tests/std/test_arena.zc
import "std.zc"
import "std/arena.zc"
test "test_std_arena" {
let arena = Arena::new(4096);
assert(arena.bytes_used() == 0, "Arena should not have used any bytes yet")
assert(arena.bytes_free() == 4096, "Arena bytes_free should be 4096 bytes")
// Allocate integers
let arr: int* = arena.alloc_n<int>(10);
for (let i = 0; i < 10; i = i + 1) {
arr[i] = i * i;
}
assert(arena.bytes_used() == 40, "Arena should have used 40 bytes")
assert(arena.bytes_free() == 4056, "Arena bytes_free should be 4056 bytes")
// Copy string
let _ = arena.dup_str("Hello, Arena!");
assert(arena.bytes_used() == 54, "Arena should have used 54 bytes")
assert(arena.bytes_free() == 4042, "Arena bytes_free should be 4002 bytes")
// Save/restore
let mark = arena.save();
assert(mark == 54, "Arena mark (savepoint) should be at 54 bytes")
arena.alloc_n<int>(100);
assert(arena.bytes_used() == 456, "Arena should have used 456 bytes")
assert(arena.bytes_free() == 3640, "Arena bytes_free should be 3640 bytes")
arena.restore(mark);
assert(arena.bytes_used() == 54, "Arena after savepoint restore should have used 54 bytes")
assert(arena.bytes_free() == 4042, "Arena bytes_free should be 4042 bytes")
arena.reset();
assert(arena.bytes_used() == 0, "Arena after reset should have used 0 bytes")
assert(arena.bytes_free() == 4096, "Arena bytes_free should be 4096 bytes")
arena.free();
}
Burnett01
left a comment
Burnett01
left a comment
There was a problem hiding this comment.
Good job on the changes!
Final review and decision is left to Zuhaitz :)
|
You make the assumption that an There is also the issue of alignment. If you align to single bytes, which is what this appears to do, then you will not be able to return pointers to values that have a larger alignment if the pointer is not aligned. At least on some architectures (Aarch64, IIRC). So you probably want to add an alignment param to the API. Malloc usually punts and returns pointers that are aligned on 16-byte boundaries. You could do that here too if you do not want the complexity of passing an alignment at every call site. It will waste a bit of memory on some allocations, but that may be preferable to making all callers deal with alignment. All of this makes it a trickier to determine the amount of space that should be used in an allocation in a generic way that is platform agnostic. These are things I've run into implementing arenas in C. I have the bruises and singed knuckles to prove it :-( It is really great to see all this hard work creating such a nice foundations for Zen-C! |
|
Thank you for your great review insights @kyle-github ! This is very valuable especially when it comes to portability. |
3 participants
No description provided.