Conversation
There was a problem hiding this comment.
Code Review
This pull request refactors the lua_embed module to implement a unified, group-based data model. The previous fixed preload and data attributes are replaced by a flexible data table where each key represents a named group, which is then generated as a field in a lua_embed_bundle struct. The generation process now dynamically creates lua_embed_data.h to match the user's configuration. Corresponding updates were made to the bee_glue.c runtime, the Ninja build logic in writer.lua, and the project documentation. Review feedback identified a risk of compilation failure or runtime crashes in bee_glue.c if standard groups like data or preload are omitted, as well as a potential path-handling bug in the generator script when no directory separator is present in the output path.
There was a problem hiding this comment.
Pull request overview
此 PR 将 lm:lua_embed 从“preload/data/main + 函数查询”的嵌入模型重构为“按命名 group 组织 + 生成全局结构体访问”的模型,并同步更新 bee.lua 胶水层与文档说明。
Changes:
- 将嵌入数据改为按
data.<group>组织,生成lua_embed_data.h+ 全局lua_embed结构体供 C/C++ 直接访问 - 更新 bee 胶水层:约定使用
lua_embed.preload/main/data三个 group 完成_PRELOAD注入、入口脚本与bee.embed - 更新/迁移文档与最佳实践示例,删除旧的
lua_embed.h
Reviewed changes
Copilot reviewed 9 out of 10 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| skills/references/core/advanced_api.md | 更新 lm:lua_embed 的 group 规则、bee_glue 约定与新的 C 侧访问方式文档 |
| skills/references/best_practices/bp_dependency.md | 将示例迁移到新的 data.<group> 结构 |
| skills/references/best_practices/bp_codegen.md | 更新 lm:lua_embed 的推荐用法与分组/bytecode 说明 |
| skills/SKILL.md | 更新 lm:lua_embed 简述与示例 |
| scripts/writer.lua | 调整 Ninja 产物为 .c + lua_embed_data.h,并更新导出 objdeps 的聚合目标 |
| scripts/lua_embed/lua_embed_gen.lua | 重写生成器:按 groups 生成条目数组、lua_embed 全局结构体与 lua_embed_data.h |
| scripts/lua_embed/lua_embed.h | 删除旧 runtime 头文件(函数式查询 API) |
| scripts/lua_embed/bee_glue.c | 适配新数据结构:从 lua_embed.<group> 直接遍历/取 main,并实现 bee.embed |
| scripts/lua_embed.lua | 重写 config 生成与输入收集逻辑,按 data 的 string key 生成有序 groups |
| .gitignore | 忽略 /.claude/ |
Comments suppressed due to low confidence (1)
scripts/writer.lua:916
use_bee目前用local_attribute.bee_glue ~= nil判断会导致bee_glue = false也启用 bee_glue.c(与文档的 bool 语义不一致)。建议改为仅在bee_glue == true时启用,并在启用时校验data.preload/data.main/data.data组存在(否则会因生成的lua_embed_bundle缺少字段而编译失败)。
local use_bee = local_attribute.bee_glue ~= nil
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
|
/gemini review |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 9 out of 10 changed files in this pull request and generated 3 comments.
Comments suppressed due to low confidence (1)
scripts/writer.lua:916
bee_glueis now documented/used as a boolean, but lua_embed treats any non-nil value as “enabled” (bee_glue ~= nil). That means an explicitbee_glue = false(or legacybee_glue = "main.lua") will still enable the glue path and trigger required-group assertions. Consider validatingbee_glue's type/value (e.g., requiretrue), and emitting a clearer migration error for the old string form.
local use_bee = local_attribute.bee_glue ~= nil
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Code Review
This pull request refactors the lua_embed system to use a flexible, group-based architecture. The previous fixed structure of preload, data, and main has been replaced by a unified lua_embed_bundle struct that supports custom groups defined in the build configuration. Key changes include updates to the generation script to produce a dedicated header file, modifications to the bee_glue layer to interface with the new data layout, and comprehensive documentation updates. Review feedback suggests optimizing the linear search used for data lookups in large sets and improving the robustness of C identifier generation to prevent potential filename collisions.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request refactors the lua_embed system to use a flexible, group-based configuration for embedding resources, replacing the previous fixed preload/data structure. It introduces a generated lua_embed_data.h header and a lua_embed_bundle struct for accessing embedded data, while removing the static lua_embed.h. The code generator has been improved with better C identifier collision handling and directory scanning logic that mirrors Lua's package.path priority. Feedback highlights a critical regression in bee_glue.c where the preloading environment is no longer initialized in the main entry point, and suggests a more robust approach for generating unique C identifiers in the generator script.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 9 out of 10 changed files in this pull request and generated 3 comments.
Comments suppressed due to low confidence (2)
scripts/lua_embed/bee_glue.c:115
_bee_main()no longer calls_bee_preload_module(). This means running the main script will not registerlua_embed.preloadintopackage.preloadnor registerbee.embedunless callers remember to call_bee_preload_module()separately, which contradicts the documented “bee_glue auto inject” behavior and is a behavioral regression from the prior implementation.
LUA_EMBED_EXPORT int _bee_main(lua_State* L) {
const lua_embed_entry* m = lua_embed.main;
if (m == NULL || m->name == NULL) {
lua_pushstring(L, "lua_embed: no main entry configured");
return lua_error(L);
}
if (luaL_loadbuffer(L, m->data, m->size, m->name) != LUA_OK)
return lua_error(L);
if (lua_pcall(L, 0, 0, 0) != LUA_OK)
return lua_error(L);
return 0;
scripts/writer.lua:916
bee_glueis documented/used as a boolean flag (bee_glue = true), but the implementation enables it whenever the field is merely non-nil. This makesbee_glue = falsestill linkbee_glue.cand enforce bee_glue group constraints, which is surprising and hard to diagnose. Consider treating it as enabled only whenbee_glue == true(or explicitly supporting legacy string values and rejectingfalse).
local use_bee = local_attribute.bee_glue ~= nil
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request refactors the lm:lua_embed system to support multiple named data groups, unified file scanning, and automatic generation of both C source and header files. Key changes include a more robust C identifier generation strategy with collision handling in lua_embed_gen.lua, and a simplified integration workflow in writer.lua that removes manual header copying. Feedback identifies a critical regression in bee_glue.c where the automatic module registration call was omitted in _bee_main, and points out the use of local absolute paths in the newly added documentation.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 10 out of 11 changed files in this pull request and generated 5 comments.
Comments suppressed due to low confidence (1)
scripts/lua_embed/lua_embed_gen.lua:30
readfile()usesf:read("a"), which is not a valid Lua file read format in standard Lua I/O (should be"*a"to read the entire file). As written, the generator will fail at runtime when trying to read any embedded input file.
local function readfile(path)
local f, err = io.open(path, "rb")
if not f then error("cannot open: " .. path .. " (" .. tostring(err) .. ")") end
local data = f:read("a")
f:close()
return data
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
No description provided.