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 enhances Xmake's C++ build capabilities by adding support for Microsoft Visual C++'s Dynamic Debugging feature. This allows developers to debug optimized builds more effectively by automatically configuring the necessary compiler and linker flags, while also managing potential conflicts with other build settings like Link-Time Optimization. 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 adds support for MSVC's new C++ Dynamic Debugging feature. The implementation is well-structured, adding a new policy and a corresponding configuration script. The script correctly checks for platform, architecture, and toolchain version requirements.
My review includes a critical fix for policy checking logic to ensure target-level settings correctly override project-level ones. I've also suggested renaming the new policy for better consistency with existing MSVC-specific policies, which involves changes in two files. Overall, these are great additions to xmake.
| end | ||
|
|
||
| -- check incompatible optimizations | ||
| local has_lto = target:policy("build.optimization.lto") or project.policy("build.optimization.lto") |
There was a problem hiding this comment.
The logic for checking if LTO is enabled is incorrect. target:policy(...) or project.policy(...) does not correctly handle the case where LTO is disabled on the target but enabled on the project. If target:policy("build.optimization.lto") returns false, has_lto will incorrectly be assigned the value of project.policy("build.optimization.lto"). You should use the same pattern as for checking the dynamic_debugging policy to correctly respect target-level overrides.
local has_lto = target:policy("build.optimization.lto")
if has_lto == nil then
has_lto = project.policy("build.optimization.lto")
end
| -- Set the default vs runtime, e.g. MT, MD | ||
| ["build.c++.msvc.runtime"] = {description = "Set the default vs runtime.", type = "string", values = {"MT", "MD"}}, | ||
| -- Enable C++ Dynamic Debugging for MSVC (requires MSVC toolset 14.44+, x64 only, incompatible with LTCG/PGO/OPT-ICF). | ||
| ["build.c++.dynamic_debugging"] = {description = "Enable C++ Dynamic Debugging for MSVC (requires MSVC toolset 14.44+, x64 only, incompatible with LTCG/PGO/OPT-ICF).", type = "boolean"}, |
There was a problem hiding this comment.
For consistency with other MSVC-specific policies like build.c++.msvc.runtime, it would be better to name this policy build.c++.msvc.dynamic_debugging. This makes it clearer that this is an MSVC-only feature. I've added another comment to update its usage in dynamic_debugging.lua.
["build.c++.msvc.dynamic_debugging"] = {description = "Enable C++ Dynamic Debugging for MSVC (requires MSVC toolset 14.44+, x64 only, incompatible with LTCG/PGO/OPT-ICF).", type = "boolean"},
| local enabled = target:policy("build.c++.dynamic_debugging") | ||
| if enabled == nil then | ||
| enabled = project.policy("build.c++.dynamic_debugging") |
There was a problem hiding this comment.
To match the suggested policy name change to build.c++.msvc.dynamic_debugging for better consistency, please update the policy name here as well.
local enabled = target:policy("build.c++.msvc.dynamic_debugging")
if enabled == nil then
enabled = project.policy("build.c++.msvc.dynamic_debugging")
#6258