Conversation
Summary of ChangesHello @luadebug, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request refines the process of generating CMake import files by implementing platform-specific logic for determining pointer byte sizes. The primary impact is on WebAssembly (WASM) targets, where the system now correctly identifies whether the pointer size should be 4 or 8 bytes based on the target architecture, leading to more accurate and compatible build configurations. Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
There was a problem hiding this comment.
Code Review
This pull request updates how the pointer size is determined for the wasm platform when generating CMake import files. Instead of relying on target:check_sizeof, it now uses a hardcoded value based on the architecture. My review includes a suggestion to simplify the architecture check to improve code readability and maintainability.
| -- get the builtin variables | ||
| function _get_builtinvars(target, installdir) | ||
| local target_ptrbytes | ||
| if target:is_plat("wasm") then |
There was a problem hiding this comment.
only for wasm? only wasm32/wasm64 archs exist.
There was a problem hiding this comment.
I expected target_ptrbytes = target:check_sizeof("void*") to work for wasm as well, but it looks like did not return nor 4 nor 8 for wasm build, so I placed problematic platforms here.
There was a problem hiding this comment.
or we should fix check_sizeof to support wasm build
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request addresses an issue with generating CMake configuration files for the wasm platform by providing a specific implementation for determining the pointer size, which avoids calling target:check_sizeof. It also includes an optimization for native builds. The changes are a good improvement, but I have one suggestion to make the logic for the wasm platform more robust and future-proof.
Despite we fixed issues with cross auto-generated CMakeConfigs, we have acquired new issue with wasm platform.
#6880
xmake-io/xmake-repo#8312 (imgui)