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.

Sign up for GitHub

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

Jump to bottom

Add Lua language support. #177

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Add Lua language support. #177

wants to merge 1 commit into from

Conversation

bocharov
Copy link

@bocharov bocharov commented Aug 10, 2023 •
edited
Loading

Lua FFI has a few notable differrences from regular generated C headers:

  • Custom prelude/epilogue with ffi.cdef directive.
  • Constants defined using static const syntax instead of #define.
  • Different comments format using -- or [[ instead of /* */.
  • Doesn't support #include, #ifdef or other macros.

Lua FFI described in more details at http://luajit.org/ext_ffi.html

Other changes:

  • Added const generics support for the structs.
  • Added implementation for array of any const size N.
  • Added try_as_str implementation for str_ref.

@bocharov
Copy link
Author

@danielhenrymantilla could you please give this pull request a review? We at Cloudflare are looking forward to your feedback. Thanks!

@bocharov bocharov force-pushed the lua-language-support branch 3 times, most recently from 54cb34f to 96530c3 Compare August 18, 2023 21:36
@danielhenrymantilla
Copy link
Collaborator

Wow, thanks for the PR! Will look into this in a few days, hopefully (feel free to ping me should I forget to do so). There is still a bit of tech debt in safer-ffi (_e.g., LegacyCType) which I've been wanting to get rid of before being able to properly add new languages here, so let's see if I can do it for this one; would a patch and/or git dependency allow you/Cloudflare to stall until I do so?

@danielhenrymantilla danielhenrymantilla added the K-feature Kind: proposed new code to implement new behaviour label Aug 21, 2023
@bocharov
Copy link
Author

@danielhenrymantilla Thanks for considering the PR!

I followed existing patterns with LegacyCType, so hopefully refactoring should be straightforward. I also pushed changes to address minor build failures. I would appreciate a re-run of the CI builds.

A git dependency is a temporary solution for one repository at Cloudflare, but we're planning to adapt this pattern across more repositories and services. So getting this merged and released would be great.

We're looking forward to it and hope we can move on this soon. Thanks again!

punkeel
punkeel suggested changes Aug 23, 2023
View reviewed changes
Copy link

@punkeel punkeel left a comment

Choose a reason for hiding this comment

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

High level review, I am not familiar with safer-ffi.

src/headers/languages/lua.rs Show resolved Hide resolved
src/headers/languages/lua.rs Outdated Show resolved Hide resolved
src/headers/languages/lua.rs Show resolved Hide resolved
src/headers/languages/lua.rs Show resolved Hide resolved
src/headers/languages/lua.rs Outdated Show resolved Hide resolved
src/headers/templates/lua/epilogue.lua Show resolved Hide resolved
src/layout/_mod.rs Show resolved Hide resolved
.github/workflows/gh-pages.yml Show resolved Hide resolved
ffi_tests/tests/main.rs Show resolved Hide resolved
@bocharov
Copy link
Author

@danielhenrymantilla any chance you could have a look at this PR? I'd appreciate any suggestions or feedback on it. Thanks!

@bocharov
Copy link
Author

Hey @danielhenrymantilla! I've updated PR and resolved conflicts with fresh master branch. Could we please move forward with this PR? Thanks!

@punkeel
Copy link

punkeel commented Oct 12, 2023

Friendly bump @danielhenrymantilla 👀

@migueldemoura
Copy link

Hey @danielhenrymantilla, is there's anything else we can help with to get this PR to a mergeable state? Thanks!

@janrueth
Copy link

I'd also be interested in this! @danielhenrymantilla

@bocharov
Copy link
Author

Any update on this?

@bocharov
Add Lua language support.
43c8a72 
Lua FFI has a few notable differrences from regular generated C headers:
* Custom prelude/epilogue with `ffi.cdef` directive.
* Constants defined using `static const` syntax instead of `#define`.
* Different comments format using `--` or `[[` instead of `/* */`.
* Doesn't support `#include`, `#ifdef` or other macros.

Lua FFI described in more details at http://luajit.org/ext_ffi.html

Other changes:
* Added const generics support for the structs.
* Added implementation for array of any const size `N`.
* Added `try_as_str` implementation for `str_ref` and `slice_ref<u8>`.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@punkeel punkeel punkeel approved these changes

@migueldemoura migueldemoura migueldemoura approved these changes

Assignees
No one assigned
Labels
K-feature Kind: proposed new code to implement new behaviour
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@bocharov @danielhenrymantilla @punkeel @migueldemoura @janrueth