Conversation
Summary of ChangesHello @waruqi, 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 refactors Xmake's toolchain detection logic on Windows by centralizing Visual Studio environment setup into a new utility module. This change streamlines the integration of various compilers (Clang, Intel, MSVC) with the MSVC toolchain, reducing code duplication and improving maintainability. Highlights
Existing toolchain check files (
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 does a great job of refactoring the Visual Studio environment detection logic into a shared utility, which significantly reduces code duplication across various toolchain check scripts. The overall approach is solid.
I've found one critical issue and a couple of medium-severity issues that should be addressed:
- Critical Bug in
clangtoolchain: There's a bug inxmake/toolchains/clang/check.luawhere an incorrect function is passed as a callback, which will lead to a runtime error. - Diagnostic Regression: In both
clangandclang-cltoolchain checks, the logic to detect and print the compiler version has been removed. This is a regression in user-facing diagnostics, and I've suggested how to restore it.
After addressing these points, this will be a very good improvement to the codebase.
| local check = function (toolchain, vcvars) | ||
| return _check_clang(toolchain, vcvars, suffix) | ||
| end |
There was a problem hiding this comment.
This check function is defined but never used. Instead, _check_clang is passed directly as a callback to toolchain_utils functions on lines 55, 60, and 66.
This is a bug because _check_clang has the signature (toolchain, vcvars, suffix), but the callback is only invoked with (toolchain, vcvars). This will cause suffix to be nil inside _check_clang, leading to a runtime error when "clang" .. suffix is evaluated.
The check function correctly captures the suffix variable from the outer scope. Please use check instead of _check_clang for the callbacks.
For example, line 55 should be:
return toolchain_utils.check_vc_build_tools(toolchain, sdkdir, check)Similar changes are needed for lines 60 and 66.
| local result = find_tool("clang-cl", {force = true, paths = paths, envs = vcvars}) | ||
| if result then | ||
| cprint("checking for LLVM Clang C/C++ Compiler (%s) ... ${color.success}", toolchain:arch()) | ||
| end |
There was a problem hiding this comment.
The previous implementation detected and printed the version of clang-cl. This change removes that functionality, which is a regression in diagnostic output for users.
I suggest adding version = true to the find_tool options and updating the print statement to include the version if available. This provides more useful feedback to the user.
local result = find_tool("clang-cl", {version = true, force = true, paths = paths, envs = vcvars})
if result then
if result.version then
cprint("checking for LLVM Clang C/C++ Compiler (%s) version ... ${color.success}%s", toolchain:arch(), result.version)
else
cprint("checking for LLVM Clang C/C++ Compiler (%s) ... ${color.success}", toolchain:arch())
end
end
| local result = find_tool("clang" .. suffix, {force = true, paths = paths, envs = vcvars}) | ||
| if result then | ||
| cprint("checking for LLVM Clang C/C++ Compiler (%s) ... ${color.success}", toolchain:arch()) | ||
| end |
There was a problem hiding this comment.
Similar to my other comment, the version detection and printing for clang has been removed. This is a regression in diagnostic information for the user.
Please consider adding version = true to find_tool and printing the version.
local result = find_tool("clang" .. suffix, {version = true, force = true, paths = paths, envs = vcvars})
if result then
if result.version then
cprint("checking for LLVM Clang C/C++ Compiler (%s) version ... ${color.success}%s", toolchain:arch(), result.version)
else
cprint("checking for LLVM Clang C/C++ Compiler (%s) ... ${color.success}", toolchain:arch())
end
end
#6893