Why am I getting a seg fault here

[waxen cipher]

https://github.com/RohanVashisht1234/zorsig/blob/main/src/morse.zig

please help with code review

[final quest]

well A, you shouldnt recreate the hashmap at runtime every time you call the function, it can just be a switch statement
and B, you deinit the hashmap before you return so accessing anything in the returned hashmap is illegal behavior, thats why it segfaults

[waxen cipher]

I thought defer can be called whenever?

[final quest]

wdym?

defer foo() means that the compiler will insert a foo() at the end of the scope

https://ziglang.org/documentation/master/#defer

[waxen cipher]

So, how to de-init the allocator after the return statement?
Should I take an allocator as an input in the function?

[final quest]

1, youd have the caller do it
2, yeah, any function that allocates should take an allocator

but again, please use a switch statement instead of that hashmap
itll be easier for both the user of the api and you as the developer
https://ziglang.org/documentation/master/#switch

[waxen cipher]

thanks for the recommendation, in production, i'll do this, but for now, I am trying to learn zig, How to take an allocator as a function input, what will be its type?

[final quest]

you'd want to take in an std.mem.Allocator
very rarely youd want to enforce that the user passes a specific allocator (i.e std.heap.ArenaAllocator) and then you could take that, but youll very rarely need to do that

[waxen cipher]

page allocator?

[final quest]

oh and i'd recommend reading
https://ziglang.org/documentation/master/#Memory

[final quest]

std.heap.page_allocator is an instance of std.mem.Allocator

[waxen cipher]

is this ok?

    const allocator = std.heap.page_allocator;
    const vals = try getHashMap(allocator);
    defer vals.deinit();

yes, it is!!

[waxen cipher]

and do I need to use defer? if I am manually putting the deinit at the end?

[final quest]

you don't but it's still good practice to use defer whenever you allocate

oh and also look into errdefer
thats what you should use in functions like getHashMap that return what they allocate instead of a defer

[waxen cipher]

ok, thanks a lot Jodi

[final quest]

its like a defer but it only runs if theres an error
that way you can clean everything up if something fails but leave it allocated if it succeeds

[waxen cipher]

why do you think hashmap should not be used? I think its just a bad overhead right? a switch statement could be much much more performant.

and what if I have implemented:

if 'A' then ".-"

and also want to do:

if ".-" then 'A'

[final quest]

a hashmap is fine to use (especially if you need to do key -> value as well as value -> key like you said, but re-creating it each time is not very efficient or practical

zig actually has a good solution for this with std.StaticStringMap

you can make the hashmap comptime known and map morse code to a u8 corresponding to the letter

no need to allocate it

[waxen cipher]

and which one is better switch or if-else?

[final quest]

switch would be better when you can use it (you cant switch on strings, but 'A' -> ".-" works)

theoretically both are as efficient but switch cases are a lot nicer to read

[waxen cipher]

is this possible? ".-" -> 'A'

[final quest]

const MAP = std.StaticStringMap.initComptime(.{
  .{".-", 'A'},
  .{"-...", 'B'},
  // etc
});

[final quest]

not with a switch statement

ish

there are neat tricks to get around it but generally speaking, you cant switch on strings

only integers and things that are represented by integers like enum tags

you could do something like this but it wouldnt be very ergonomic in this case:

const MorseMapping = enum {
  @".-" = 'A',
  @"-..." = 'B',
  // ...
};

const some_morse_string = "-...";
std.debug.assert(std.meta.stringToEnum(MorseMapping, some_morse_string).? == 'B');

[waxen cipher]

ok, thanks

[final quest]

and that's using the @"" identifier syntax (https://ziglang.org/documentation/master/#Identifiers)
and the .? operator (https://ziglang.org/documentation/master/#Optionals, https://ziglang.org/documentation/master/#Table-of-Operators)

and std.meta.stringToEnum is here: https://ziglang.org/documentation/master/std/#std.meta.stringToEnum

[waxen cipher]

oh wow:
https://github.com/RohanVashisht1234/zorsig/blob/main/src/morse.zig

this is also working 🤣

[final quest]

I meannn
You could do it like that lol

I personally wouldnt

Also might be very slow, especially the other way around
Since a string comparison chain cant be easily optimized to a jump table

[waxen cipher]

the static string map would be good right?

[final quest]

Yeah thats definitely how id do it

Definitely the easiest to write/read and most efficient way that i can see

Although technically the character to morse string conversion could be optimized to a table
Which would be faster than using a hashmap even if its a StaticStringMap
I really wouldnt worry about that rn though

[waxen cipher]

what do you think is the issue here:
https://github.com/RohanVashisht1234/zorsig/blob/main/src/morse.zig#L12

[final quest]

Data doesnt have enough space to fit everything
Right now its 0 bytes big because it has no elements and youre trying to fit an arbitrarily large string into it

there are multiple ways to deal with it but the easiest is to use an std.ArrayList
Which is an automatically-expanding array
So you can append to it as much as youd like

pub fn morse_to_string(allocator: Allocator, string_of_keys: []const u8, delimiter: []const u8) ![]const u8 {
    var it = std.mem.split(u8, string_of_keys, delimiter);
    var list = std.ArrayList(u8).init(allocator);  
    while (it.next()) |morse| {
      try list.append(morse_to_char(morse));        
    }
    return list.toOwnedSlice();
}

[waxen cipher]

how about:

pub fn morse_to_string(string_of_keys: []const u8, delimiter: []const u8) []const u8 {
    var it = std.mem.split(u8, string_of_keys, delimiter);
    var i:u8 = 0;
    var data: [30000]u8 = [1]u8{
            0,
        } ++ [1]u8{0} ** 29999;
    while (it.next()) |x| : (i += 1)  {
        data[i] = morse_to_char(x);
    }
    return data[0..i];
}

[final quest]

that would work as long as there are less than 30000 characters

so it could be a compromise yeah

well actually no

since youre returning a slice, which is a pointer to data
and data becomes invalidated as soon as the function returns

so youd need to return the entire 30_000 byte array AND a length

or you could return a std.BoundedArray, which is essentially just an array + a slice

pub fn morse_to_string(string_of_keys: []const u8, delimiter: []const u8) !std.BoundedArray(u8, 30_000) {
    var it = std.mem.split(u8, string_of_keys, delimiter);
    var list: std.BoundedArray(u8, 30_000) = .{};
    while (it.next()) |x| {
      try list.append(x);
    }
    return list;
}

but imo that's not very elegant for the user

[waxen cipher]

yes, is it fine that I can call you here whenever I have something to ask, is that ok? or a new post?

[final quest]

id prefer not to call
a post would be okay, but there are plenty of other people here to help you too, you shouldnt single me out

[final quest]

also I would probably make it a MorseDecodeIterator or something like that, that way you could iterate it just like the SplitIterator

[waxen cipher]

ok, thanks a lot jodi

[final quest]

np!

[gentle mantle]

I'd also note that 30,000 bytes on the stack is much more than I'd put there.
I generally have a rule of thumb that any individual thing on the stack is no more than 4,000 bytes.
Stack space is very limited and your entire program uses it.

But yes, you can pass an output buffer into the function (define an array in the caller and pass &data into the function), or you can allocate on behalf of the caller with an allocator that they give you.

[waxen cipher]

I think jodi's fix is the best for array list in that case

[final quest]

30,000 bytes isnt a lot for the stack
You have at least 1MiB usually
And its a short lived function

The issue for me would be the copying and that itd take up a lot of cache lines

[gentle mantle]

The arraylist is indeed a decent path; it's basically just the "allocate on behalf of the caller" anyway

[final quest]

Id really recommend using an iterator for this
Where each next() call decodes the next character and returns it

[waxen cipher]

is there a nice guide about wasm with zig?

and can js call the pub fn ? without exporting them?

[final quest]

Youd need to export them

[waxen cipher]

I think exported function can't take []const u8 as input

[final quest]

but also it does just kinda work if you just set the target to wasm32
It gets a bit more difficult if you need libc since youd need to mess with emscripten but if you dont its not bad at all

[final quest]

Yeah it cant
Slices dont have a stable abi
Just take a pointer and a length

[waxen cipher]

[*c]const u8?

[gentle mantle]

[*]const u8

[waxen cipher]

and to access the data inside it I can &[*]const u8?

[gentle mantle]

No

You just index or slice that

const slice = that_ptr[0..n];

[waxen cipher]

ok, also I just need a nice guide for zig for wasm

any links?

[gentle mantle]

You only need to use & if you want to get the address of something
But [*]const u8 is a multi-pointer, which is already a pointer type.

[final quest]

zig doesnt do anything special with wasm
So just see the official guide for using wasm within JS and itll work
In zig, you just set the target to wasm32 and export the symbols you want to be available

[waxen cipher]

and can these exported functions return []const u8?

[final quest]

no

JS has no idea what a []const u8 is

[waxen cipher]

then [*]const u8 needs to be returned?

or, an array of characters?

[final quest]

thats one way to do it, but youd also need to provide a length for it to be useful
or return it null terminated I guess

again, iterator would probably work the best here
that way you dont need to worry about how youd store anything

[waxen cipher]

thanks

[final quest]

you can also look at the page for it in the language reference
its small but it can get you started
https://ziglang.org/documentation/master/#WebAssembly

[gentle mantle]

You cannot really return an array of characters to C with the return value; as Jodi said, you'd need to return the length as well anyway, or return a null-terminated pointer instead.
Both can work but it's more manual than just returning a Zig slice.
I would probably do

const char *the_function(size_t *out_count);

fn the_function(out_count: ?*usize) [*]const u8 {
    ...

    const result: []const u8 = ...;
    if (out_count) |*n| n.* = result.len;
    return result.ptr;
}

[final quest]

yeah thats a good way to do it

[gentle mantle]

Or you can make a extern struct which has both the pointer and length, and return one of those or do a similar out-pointer thing

But doing it as in that example is simpler to do I suspect

[final quest]

unfortunately that doesnt work with wasm
the only way to really pass structs between wasm and JS is using some serialized format like JSON

or ofc just a lot of out parameters lol

[gentle mantle]

Yeah - returning structs in general in C seems iffy

[final quest]

nah it's fine in C

as long as it's extern

[gentle mantle]

Both Odin and Zig had trouble implementing that correctly

[final quest]

I assume it works fine now, at least in zig since its such a staple feature

[gentle mantle]

Well sure - but it's indicative of the complexity is what I mean

[final quest]

yeah

[gentle mantle]

Not that it always matters to any one programmer specifically

But we're trying to make good software here 😄

[final quest]

unfortunately systemv abi is pretty icky when it comes to returning structs
theres a lot of rules

[gentle mantle]

Yeah exactly

[final quest]

idk about stdcall but I assume its the same way on windows

either way, you should feel fine using it now

[gentle mantle]

I forget, but I feel like stdcall uses an outpointer for that in the ABI

Might be wrong though

[gentle mantle]

FWIW, I think that attitude---while makes sense and is generally fine enough---with stuff like this, I am concerned that it leads to wasting other people's time in the longrun.

I want people to be able to use code with confidence without running into 10 different subtle issues that they have to workaround first

Relying on those complex ABI rules has questionable worth unless you can measure the speed difference to an out-pointer, [etc,] in a specific usecase I feel like

And besides which - structs require [more] bindings.

So it's more annoying to use the API as well

More work required at least

[waxen cipher]

I just realized https://github.com/RohanVashisht1234/zorsig/blob/main/src/zorsig.zig#L12
Is the memory getting de-initialized here? ik that returning list.toOwnedSlice() doesn't require a de-init, but still?

[gentle mantle]

If I'm not mistaken, toOwnedSlice returns the contents of the list, or a copy of it if the allocator doesn't allow resizing.

I generally put an errdefer list.deinit(); in situations like you have there

[gentle mantle]

See - that's what I generally say too

I generally don't deinit if I'm returning the thing

Because it's confusing

I generally errdefer instead

[waxen cipher]

then, is the memory getting deinitialized?

[gentle mantle]

Just checked the code

Nevermind

In the case where it returns a copy

It frees the list contents and clears it anyway

[gentle mantle]

So I will second this advice. ^

In your example, if any of the trys fail, the list is leaked for example

[waxen cipher]

how about this:

pub fn morse_to_string(allocator: std.mem.Allocator, string_of_keys: []const u8, delimiter: []const u8) ![]const u8 {
    var it = std.mem.split(u8, string_of_keys, delimiter);
    var list = std.ArrayList(u8).init(allocator);
    while (it.next()) |morse| try list.append(morse_to_char(morse));
    const x:[]const u8 = try list.toOwnedSlice();
    defer list.deinit();
    return x;
}

[gentle mantle]

That doesn't do anything more than the previous example

toOwnedSlice clears the list

And that leak I mentioned can still happen

[waxen cipher]

ok, so I'll defer deinit when there is an error

[gentle mantle]

Right

errdefer is your friend there

[waxen cipher]

thanks

[final quest]

you should probably deinit list.deinit() right after you create list
otherwise it's basically useless

you dont even need to errdefer here

[waxen cipher]

*defer

[final quest]

yeah my bad

[final quest]

fwiw, deinit() after toOwnedSlice still valid there, its just a no-op
but yeah errdefer is probably better

[waxen cipher]

earlier you told me that we shouldn't return defer de-initialized stuff, because it will crash?

[final quest]

also here's an iterator approach if it helps

const MorseDecoderIterator = struct {
  split_iter: std.mem.SplitIterator(u8, .sequence),
  pub fn next(iter: *MorseDecoderIterator) ?u8 {
    const string = iter.split_iter.next() orelse return null;
    return morse_to_char(string);
  }
  pub fn init(morse: []const u8, delim: []const u8) MorseDecoderIterator {
    return .{
      .split_iter = std.mem.splitSequence(u8, morse, delim),
    };
  }
};

super simple right?

[final quest]

I dont think I said that, you mightve misinterpreted it
but that is true
but deinit on ArrayList is defined to do nothing if it was already cleared
so it becomes a no-op instead of deinitializing it again

no-op meaning "no operation" aka it does nothing

[waxen cipher]

I was talking about the hash map crash

[final quest]

yeah, the issue there was that you were using the hashmap after you deinitialized it

which is different

that's called a UAF (Use After Free)

freeing/deinitializing data that was already freed is called a Double Free

theyre two different things

but both are bad ofc

[waxen cipher]

the B part here

but after defer deinit, accessing anything in the array list is legal? and illegal in the hashmap?

[waxen cipher]

this is perfect, thanks

[gentle mantle]

It is, but the ship has sailed on it being confusing as far as I'm concerned

Hence errdefer

[final quest]

defering it means that deinit only runs at the end of the scope (if it errors with try or if you return)
but after that deinit runs, its illegal to access it

[gentle mantle]

I do agree though to be clear that deinit on an empty/emptied thing doing nothing is a nice feature though

That kind of thing generally is helpful for stuff like conditional allocations

[Freeing empty slices is a no-op for the same reason]

[waxen cipher]

then how am I legally still accessing list.toOwnedSlice()?

after defer deinit and forcefull deinit

[final quest]

toOwnedSlice re-allocates it
so it's valid until you free that new slice

which is up to the caller to do

(theyd need to do allocator.free(slice) to the slice they get back from that function)

[waxen cipher]

oh! thanks

[gentle mantle]

Yeah - toOwnedSlice's job is basically just the same as using list.items, except ensuring that list.items.len is the entire allocation instead of just part of it.

[final quest]

you can read the source code for it, id highly recommend doing that whenever you encounter a function
even if it seems simple, it'll help you learn quite a lot in my experience
https://ziglang.org/documentation/master/std/#std.array_list.ArrayListAligned.toOwnedSlice

[gentle mantle]

you can read the source code for it, id highly recommend doing that whenever you encounter a function
even if it seems simple
I second this wholeheartedly.
You're ultimately responsible for the code you are using whether you wrote it or not; it stands to reason that you should understand it 😁

On top of the fact that reading the source is generally also a good way to learn a lot, as noted.

[waxen cipher]

Perfect

Are there any cross platform fully compatible gui libraries available for zig rn?

[final quest]

you can use anything for C pretty comfortably in zig
and there's also Capy which ive never used but it seems to be relatively popular

https://github.com/capy-ui/capy

[waxen cipher]

Capy is planned for macos hence it's not available atm

I needed anything that seems ready for use

I think the only thing available for current use is webui

[final quest]

yeah or just use gtk/raygui/nuklear/cimgui/etc from C

you really can use any C gui library

webui is very cool though
I wouldnt use it if a gui is reguired but as an optional gui component of a command line app, it works very well

[gentle mantle]

DearImgui is something I've heard decent things about - at least from gamedevs

MicroUI might also be enough depending on what you need and want

But I've not used either of them myself yet so

[final quest]

dear imgui is kinda icky to get working with zig since it's a C++ library and cimgui has almost no exmaples or documentation

[gentle mantle]

No idea how so many use it then

People tend to reach for in Odin

AFAICT

[waxen cipher]

Hello!, why is this an unsupported os error?
https://github.com/RohanVashisht1234/zorsig/blob/main/src/zorsig.zig#L30

[quick sphinx]

if that map is static and you're using 0.13 you might instead use a std.StaticStringMap(T).initComptime() which doesn't require allocation and can. not sure if that would address the error.

seems like the return type for that fn is just wrong. should return a std.StringHashMap([]const u8) i think

[waxen cipher]

just refresh the page

[quick sphinx]

this line in the test below looks wrong

 _ = get_hash_map_char_to_morse(std.heap.HeapAllocator);

i would expect something like

const map = try get_hash_map_char_to_morse(std.testing.allocator);
defer map.deinit();

[waxen cipher]

ok, I am currently working on static map, what input does the initComptime function take?

[quick sphinx]

go here and click the [src] link. then scroll down and look at the tests https://ziglang.org/documentation/master/std/#std.StaticStringMap

[waxen cipher]

thanks

[quick sphinx]

np. you should be able to declare something like this. since this is staic memory, it can be a top level declaration.

    const map_char_to_morse = std.StaticStringMap([]const u8).initComptime(.{
    .{"A", ".-"},
    .{"B", "-..."},
    // ...
    });

EDITED

[waxen cipher]

perfect

[waxen cipher]

is this a glitch in zig?

[wind elbow]

https://ziglang.org/documentation/master/#Top-Level-Doc-Comments

[waxen cipher]

oh! its like rust

[waxen cipher]

perfect, thanks

[quick sphinx]

@waxen cipher I think you'll find that using writers is more flexible than requiring an allocator. I did a quick rewrite which does this. Plus I got rid of redundancy so there is a single source of truth. https://zigbin.io/f9c6a5

i've updated the test at the bottom too

and confirmed that it passes

[waxen cipher]

so, basically, you removed the if else and are directly using the hash map?

[quick sphinx]

yes

[waxen cipher]

is that performant?

[quick sphinx]

StaticStringMap() is quite performant. but i'm not sure what is quicker. if you want to build up an if-else chain, i would recommend doing inline for(comptime map.keys(), comptime map.values())

this way you maintain a single source of truth

similar to how i've build up the reverse map

[waxen cipher]

ok, so basically its like:

• if we are ready to allocate memory for static purpose, why not use the allocated memory itself instead of introducing if else

[quick sphinx]

something like that. the main advantage of accepting writers instead of and allocator is that you can pass in other types of writers such as File.Writer, io.FixedBufferStream.Writer, etc

oic thats now what you're asking above...

[waxen cipher]

is this getting run at the comptime?

pub const map_morse_to_char: std.StaticStringMap(u8) = blk: {
    var kvs: [map_char_to_morse.kvs.len]struct { []const u8, u8 } = undefined;
    for (map_char_to_morse.keys(), map_char_to_morse.values(), 0..kvs.len) |k, v, i| {
        std.debug.assert(k.len == 1);
        kvs[i] = .{ v, k[0] };
    }
    break :blk std.StaticStringMap(u8).initComptime(kvs);
};

[quick sphinx]

yes. it is because its a block in global scope.

[quick sphinx]

i chose to use map.get instead of doing an inline for just because it was simpler and i didn't consider performance.

[waxen cipher]

ok, so, are allocators slower than writers?

[quick sphinx]

not necessarily. but writer is a more flexible thing which allows users to determine if they want to allocate or not

and thus choose to maybe do somthing more performant

[final quest]

depends entirely on the implementation of said writer or allocator
if you actually allocate on the heap with the Allocator, it will be very slow
but if you use the Writer interface to write to a network opened in Antartica, itll obviously also be slow
you cant really compare them without more context

[waxen cipher]

writer seems to me like we are reducing a step for ourselves and letting the user declare the array.

[quick sphinx]

yes we are giving users a choice about how to handle the memory management.

[final quest]

although generally speaking, heap allocations are one of the slowest things you can do (including the cost of the cache misses + frees)
but not all Allocators allocate on the heap and the ones that do will re-use previous allocations most of the time

[waxen cipher]

hmm this seems like, if I provide my library with both the features as seperate functions, it would be a better choice.

[final quest]

plus Allocators are type erased so they arent inlineable but that shouldnt matter usually, just kind of an implementation detail

[quick sphinx]

i think just providing the writer methods is enough. users can get the array list behavoir by doing the same as i did in the tests

which is equivalent to what you were doing before

[final quest]

you dont need to provide apis that allocate unless it’s something you can do better than the user
in this case it would be just as efficient for the user to use a writer into an ArrayList(u8)

or similar

if they wanted it to allocate

in fact it would be more efficient since you wouldnt need to use toOwnedSlice

[waxen cipher]

nice, do we have any place where we can put our libraries instead of github?

like, a place like npm/crates.io

[final quest]

nope, zig chooses not to have a central repo for packages

npm/crates.io have had a lot of issues (some of which are ongoing) that zig wouldnt want to repeat

[waxen cipher]

atleast there should be a registry

[final quest]

meh

[waxen cipher]

a place where people can browse different libs?

[final quest]

there’s #1024381264213594242 which you could say acts as a registry

and awesome-zig but its a bit outdated and generally kinda meh

[quick sphinx]

i'm certain there will be registries that pop up. just maybe not an official one

[final quest]

yeah

meghan seems to have made like 80% of zig libraries somehow, so her github profile is kind of a registry at this point

but also, you really dont need a lot of libraries in zig
the std is pretty big and it has the tools to trivially make a lot of things you might want
imo there arent that many things that really deserve to be libraries

generally its better to avoid dependencies

ofc thats just me though
not everyone is like that

[waxen cipher]

ok, thanks

[quick sphinx]

not sure how up to date these are, but this is related at least https://zig.news/xq/zig-package-aggregator-58co

[waxen cipher]

oh wow

oh no

this seems very very old

https://zig.pm

but this is actually perfect

[quick sphinx]

let us know how it goes if you try adding your package there. i have some packages maybe i should do the same.

[waxen cipher]

a pr from 20 may is pending there

[quick sphinx]

did you see this?

If you have your package online on GitHub, please add the zig-package topic to your repository.

[waxen cipher]

Yes, the zig-package or zig-library something

I am adding zeejango to it 💀

[quick sphinx]

hmm i'm not sure that actually works. i have some repos where i've added that tag and they don't show up

[waxen cipher]

It takes 15 mins

For zig-package

[quick sphinx]

i mean existing ones that have been up with that tag for a long time

i would maybe ask in #zig about registries and where to post your package

[waxen cipher]

https://zigistry.vercel.app/
Just started making this