Conversation
Summary of ChangesHello @lebao3105, 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 significantly improves Pascal language support within Xmake by introducing a comprehensive set of new configuration options and the underlying logic to translate these options into Free Pascal Compiler (FPC) specific flags. This enhancement allows users to precisely control various aspects of Pascal compilation, such as managing search paths for units and includes, defining or undefining preprocessor macros, and specifying the desired Pascal language dialect, thereby offering greater flexibility and compatibility for Pascal projects. Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. 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 improves Pascal language support by adding several new APIs for compiler flags like include paths, unit paths, defines, and language modes. The changes are mostly correct and well-implemented.
I've left a specific comment in xmake/modules/core/tools/fpc.lua to make the language mode handling more robust.
Additionally, I noticed an inconsistency in xmake/languages/pascal/xmake.lua. The new nameflags have been added to the binary target kind, but not to the shared target kind. Since fpc handles both compilation and linking in a single step, shared targets would likely benefit from these flags as well. Please consider updating the shared section for consistency.
|
Please do not add new apis. Any new API should have its design discussed in detail in issues, and a decision made on whether it is truly necessary, rather than being added arbitrarily to a PR. In addition, the design of APIs should be as abstract as possible and be universal, rather than being provided for use by a specific language such as Pascal. If it's only used for a specific language, I usually recommend using add_fcflags to set it directly. |
Known issue(s):Introduced via the code below. Flags are somehow re-addedlebao3105@ultramarine Commands-collection on master [✘!?] via 🌙
❯ XMAKE_PROGRAM_DIR=$(pwd)/../xmake/xmake xmake b -v selected
[ 27%]: archiving.release libcustcustc.a
/usr/bin/ar -cr build/linux/x86_64/release/libcustcustc.a build/.objs/custcustc/linux/x86_64/release/include/custcustc.c.o
[ 54%]: linking.release dir
/usr/bin/fpc -dbg:=begin -ded:=end -dretn:=procedure -dfn:=function -dlong:=longint -dulong:=longword -dint:=integer -dbool:=boolean -dreturn:=exit -Fiinclude -Fuinclude -Mfpc -O3 -Sa -Si -Sm -Sc -Sx -Co -CO -Cr -CR -gl -k-lc -k-Lbuild/linux/x86_64/release -Xs -dbg:=begin -ded:=end -dretn:=procedure -dfn:=function -dlong:=longint -dulong:=longword -dint:=integer -dbool:=boolean -dreturn:=exit -Fiinclude -Fuinclude -Mfpc -k-lcustcustc -obuild/linux/x86_64/release/dir src/dir/dir.pp
error: Free Pascal Compiler version 3.2.2 [2025/08/02] for x86_64
Copyright (c) 1993-2021 by Florian Klaempfl and others
Target OS: Linux for x86-64
Compiling src/dir/dir.pp
dir.pp(48,9) Error: Identifier not found "try"
dir.pp(49,13) Fatal: Syntax error, ";" expected but "identifier ISNAMEVALID" found
Fatal: Compilation aborted
Error: /usr/bin/ppcx64 returned an error exitcodeNotice how definitions (ones starting with Example(s)add_rules("mode.debug", "mode.release", "mode.releasedbg")
add_rules("plugin.compile_commands.autoupdate")
target("dir")
set_kind("binary")
set_languages("pascal")
add_files("src/dir/dir.pp")
add_rules("pascal")
add_deps("custcustc")
add_defines(
"bg:=begin", "ed:=end", "retn:=procedure",
"fn:=function", "long:=longint", "ulong:=longword",
"int:=integer", "bool:=boolean", "return:=exit"
) -- add -d<definition> flags
add_pcflags("-Sa", "-Si", "-Sm", "-Sc",
"-Sx", "-Co", "-CO", "-Cr", "-CR", "-gl") -- add flags into the compiler as-is
add_includedirs("include") -- add -Fi<includedir> flags
add_unitdirs("include") -- add -Fu<unitdir> flags |
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request significantly expands Pascal language support in xmake by adding new APIs and wiring them up for the Free Pascal Compiler. The changes introduce support for include directories, unit directories, defines, and other compiler/linker settings.
My review has identified a few critical issues:
- In
xmake/languages/pascal/xmake.lua, thenameflagsforobjecttargets incorrectly include linker flags, while thenameflagsforbinaryandsharedtargets are missing essential linker flags, which would break linking and dependency tracking. - In
xmake/modules/core/tools/fpc.lua, the implementation for handling exceptions is logically incorrect and will produce the opposite flag for disabling exceptions.
I've provided detailed comments and suggestions to fix these issues. Once these are addressed, this will be a great improvement for Pascal developers using xmake.
|
add_unitdirs is only for pascal, However, all abstract APIs should be able to be used in multiple languages, not just Pascal. However, all abstract APIs should be able to be used in multiple languages, not just Pascal. unitfile appears to correspond to objectfile in other languages, but currently no other language requires support for configuring search directories for individual objectfiles. |
|
done #7260 |
| , "target.add_shflags" | ||
| , "target.add_rpathdirs" -- @note do not translate path, it's usually an absolute path or contains $ORIGIN/@loader_path | ||
| , "target.add_unitdirs" | ||
| , "target.add_includedirs" |
There was a problem hiding this comment.
it will break something, we should move to path api. #7260
#7224