Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

bindgen: Initial doc comments for Odin & Zig #1176

Merged
merged 8 commits into from
Jan 11, 2025

Conversation

AlexanderArvidsson
Copy link
Contributor

@AlexanderArvidsson AlexanderArvidsson commented Jan 5, 2025

Here's a proof of concept method of preserving comments in bindgen, as mentioned in #1147.

Right now I've only added it to the Odin (and now Zig) bindgen, but each generator can enable it by turning on with_comments flag to the gen_ir parser. But after that, the generator must handle the additional AST nodes. In Odin, I did this simply by stripping away all comments whenever we're looping through inner node children, and getting the comment separately and adding it to the output.

Should be straight forward to add the same behavior to the other generators, but this is a good start!

Here's some examples of some comments:

@(default_calling_convention="c", link_prefix="sg_")
foreign sokol_gfx_clib {
    // setup and misc function
    setup :: proc(#by_ptr desc: Desc)  ---
    shutdown :: proc()  ---
    isvalid :: proc() -> bool ---
    reset_state_cache :: proc()  ---
    push_debug_group :: proc(name: cstring)  ---
    pop_debug_group :: proc()  ---
    add_commit_listener :: proc(listener: Commit_Listener) -> bool ---
    remove_commit_listener :: proc(listener: Commit_Listener) -> bool ---
    ....
    frame_stats_enabled :: proc() -> bool ---
    query_frame_stats :: proc() -> Frame_Stats ---
    // D3D11: return ID3D11Devic
    d3d11_device :: proc() -> rawptr ---
    // D3D11: return ID3D11DeviceContex
    d3d11_device_context :: proc() -> rawptr ---
    // D3D11: get internal buffer resource object
    d3d11_query_buffer_info :: proc(buf: Buffer) -> D3d11_Buffer_Info ---
    // D3D11: get internal image resource object
    d3d11_query_image_info :: proc(img: Image) -> D3d11_Image_Info ---
    ...
}
...

/*
    sg_usage

    A resource usage hint describing the update strategy of
    buffers and images. This is used in the sg_buffer_desc.usage
    and sg_image_desc.usage members when creating buffers
    and images:

    SG_USAGE_IMMUTABLE:     the resource will never be updated with
                            new data, instead the content of the
                            resource must be provided on creation
    SG_USAGE_DYNAMIC:       the resource will be updated infrequently
                            with new data (this could range from "once
                            after creation", to "quite often but not
                            every frame")
    SG_USAGE_STREAM:        the resource will be updated each frame
                            with new content

    The rendering backends use this hint to prevent that the
    CPU needs to wait for the GPU when attempting to update
    a resource that might be currently accessed by the GPU.

    Resource content is updated with the functions sg_update_buffer() or
    sg_append_buffer() for buffer objects, and sg_update_image() for image
    objects. For the sg_update_*() functions, only one update is allowed per
    frame and resource object, while sg_append_buffer() can be called
    multiple times per frame on the same buffer. The application must update
    all data required for rendering (this means that the update data can be
    smaller than the resource size, if only a part of the overall resource
    size is used for rendering, you only need to make sure that the data that
    *is* used is valid).

    The default usage is SG_USAGE_IMMUTABLE
*/
Usage :: enum i32 {
    DEFAULT,
    IMMUTABLE,
    DYNAMIC,
    STREAM,
}

These then show up in the OLS LSP, at least in neovim:
image

@floooh
Copy link
Owner

floooh commented Jan 5, 2025

Very cool! I'm currently deep into emulator stuff but as soon as I crawl out of that rabbit hole again I'll try to reserve some time to take care of pending PRs.

One thing I keep rolling around in the back of my head is to find a better doc-comment structure which doesn't generate such huge documentation panels when mousing over a struct declaration. But that's unrelated to making the comments available in the bindings code :)

Copy link
Owner

@floooh floooh left a comment

Choose a reason for hiding this comment

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

Just some quick'n'dirty review comments where I think it's worth it to move some similar code into a helper function.

bindgen/gen_odin.py Outdated Show resolved Hide resolved
bindgen/gen_odin.py Outdated Show resolved Hide resolved
bindgen/gen_odin.py Outdated Show resolved Hide resolved
@floooh
Copy link
Owner

floooh commented Jan 5, 2025

PS: don't worry about the CI error, that's something unrelated in the D bindings test.

@AlexanderArvidsson AlexanderArvidsson changed the title bindgen: Initial doc comments for Odin bindgen: Initial doc comments for Odin & Zig Jan 5, 2025
@AlexanderArvidsson
Copy link
Contributor Author

AlexanderArvidsson commented Jan 5, 2025

@floooh Added a utility function to add comment outputs (if exists), also found a better way to handle indenting and prefixing via textwrap standard library.

Also went ahead and added Zig comments, just to make sure it was ergonomic to "port" from one generator to another considering different syntax requirements. Should be much better now for porting!
But Zig is untested atm (there's some compile-time requirements for /// comments in Zig but I don't have Zig atm), I'll update here tomorrow once it's tested.

One thing I keep rolling around in the back of my head is to find a better doc-comment structure which doesn't generate such huge documentation panels when mousing over a struct declaration. But that's unrelated to making the comments available in the bindings code :)

I think it's fine, they're not unbearably big (apart from the huge top comment block which would be useful to at least have in the bindings but not attached to a specific declaration). As this comment often includes important notes, examples, etc, I think it would be useful to somehow include that in the bindings so I don't have to navigate to the C files.

@AlexanderArvidsson
Copy link
Contributor Author

Made a final change to the comments to skip writing any whitespace on empty comment lines, as some editors tend to highlight these:
image

Also confirmed Zig compiles with these comments, as it should.

This should be ready to be merged now!

@AlexanderArvidsson
Copy link
Contributor Author

I added an extremely rudimentary method to include the top comment in the output. The clang AST doesn't include it, because it's not attached to any node, so I just wrote a regex that extracted the first comment block (/* ... */).

I'm wasn't too happy with just the top comment, so I made sure to only include it if it contains the "Project URL" section (which is also not ideal, I know), which all files do. Another method would be to have a special mark (like @header_comment) inside the comment to detect it that way.

It's up to you if you want to include it or not! Let me know if I should revert that part.

@floooh
Copy link
Owner

floooh commented Jan 9, 2025

FYI: I'll try to take care of this PR after wrapping up some more PRs in my emulator project, most like on the weekend.

floooh added a commit that referenced this pull request Jan 11, 2025
@floooh floooh merged commit 81dfc1d into floooh:master Jan 11, 2025
29 of 32 checks passed
@floooh
Copy link
Owner

floooh commented Jan 11, 2025

Ok, merged! I also added an entry to the changelog. Many thanks!

@floooh
Copy link
Owner

floooh commented Jan 11, 2025

Ok, sokol-zig and sokol-odin updates are live:

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants