As a first zig project for learning the language, I ported the minimal kilo editor to zig, faithful to the original only with some better syntax highlighting, autoindent and textwidth wrapping. Others had the same idea and there are other versions on github but they are mostly incomplete or had segfaults of some sorts while trying them.
The intention now would be to make a version of Build Your Own Text Editor for zig, but I would appreciate if somebody with experience with the language took a look at the code since I wouldn’t want to write crap. Thank you in advance.
Looks good! I don’t have time for a full review, so I scanned as much of the code as I could in a few minutes. My first impression is really good. Very tidy code, easy to understand and no major issues found in a five minute read. Nice work!
Just in the order I discovered them following source
A good amount of this is just preference.
GeneralPurposeAllocator has been renamed to DebugAllocator and is quite slow so it’s recommended to use either libc’s allocator or smp_allocator for release builds
Having a types file is fine, but I’d expect the functionality of editor.zig to be in the Editor type and in its own file Editor.zig (files are structs btw, they can have fields and be instantiated) instead of the general types file.
I would have the global variables in editor be fields of Editor
if you look in createFileAbsolute you can see it just asserts the path is absolute, then calls cwd().createFile
Look into the Reader/Writer api (preferably on master as its being updated and will drop soon) for data streaming, would let you minimize allocations when reading/writing.
also the dependency injection of readLines is unnecessary, don’t abstract until you need to.
realloc copies the old data to the new allocation, if you are going to overwrite that, or only a subsection of the allocation is used, it’s better to alloc and do the rest yourself.
Allocator has a dupe function
found a couple of helper functions that just obscure a field access, encourages a lack of understanding of your data structures.
Syntax.mlcmt has a fixed number of items, so you should use an array, or a pointer to an array, instead of a slice, to enforce that.
you can use /// to turn comments into documentation that zls and zig can use, though it only works if it’s above what is being documented.
‘managed’ containers which contain their allocator are deprecated in favour of Unmanaged versions which will take the names of the 'managed` versions in future updates. Just means you will have to pass the allocator into functions that require it.
use appendNtimes instead of a loop, it will do some math for a @memset
try to allocate upfront then use non allocating api’s, more efficient and minimizes the locations errors can be returned. encouraging this is one reason Unmanaged containers are prefered.
switches can capture |t| the value they are branching on, even tagged unions which have payload |payload,tag|, you can inline .a, .b to make the tag comptime known to generate each branch individually for optimisations.
you are doing multiple syscalls for input when you could read to a larger buffer with one. This is a pattern benefited by a Reader
maybe dont shorten whitespace to whites lmao
Nothing horrible, the biggest things are just unfamiliarity with APIs.
Since you are planning to make a guide, I suggest getting familiar with std so you can teach people those, as it would be catered to those new to zig.
I’m a bit confused about this: shouldn’t realloc be cheaper than free + alloc when the final size is less than before (guaranteed resize with no copying) or even when it is only a bit bigger (could resize instead of reallocating the whole thing)? For example when I update row and syntax, changes are often minimal (and they can be negative).
your right that in that case its faster than alloc + free, but even then you could potentially avoid some branches if you do it yourself.
Performance/efficiency is a part of it, but it’s also about being explicit about how the allocation can be modified in those places in the code.
realloc is more of a convenience function IMO, it will alloc if it was 0 sized, will free if the new size is 0, will try to remap but will fall back to alloc + memcpy + free.
remap is different from resize in that it might change the location of the allocation, so it doesn’t provide pointer stability.
Doing it myself in this case would be just free + alloc instead of realloc, I would still modify/memcpy stuff on the new memory in the same way. I’m still unsure, remap should be cheaper in many cases than a full free + allocation somewhere else. But it’s just my guess. I’ll keep this change for later anyway. Thanks again.
its mostly when it does fallback to alloc + memcpy + free, you will always be able to do a more efficient memcpy the realloc can.
i wasnt intending to suggest your use of it was wrong/bad. Just that it could be depending on what you were doing.
The only inefficient uses i see are
editor.zig:696
editor.zig:1211
string.zig:18
everything else is fine
One more thing I just noticed, you are storing allocators in Row and Buffer, that’s 2 pointers of memory that could be a function parameter. Especially important for Row since it’s stored in an ArrayList, which also stores its allocator (until its unmanaged version takes its name).
also using undefined for default field values is very bad, as its literally an undefined value, forget to set it once and anything could happen, much better for the compiler to yell at you for forgetting.
It is the opposite, you are telling the compiler that you want the value to be undefined.
And there aren’t enough safety checks yet so that all ways to use such an undefined value would be caught reliably. So for now you have to be careful and it is better to only use undefined, when you are making sure that you are setting it to a valid value before you actually use it.
In cases where some field sometimes needs to be set to undefined, it is better to avoid using a field default value for that and instead pass undefined for that field value explicitly during initialization/setup.
Nice work!