Various psyqo-lua fixes & improvements#1924
Conversation
nicolasnoble
commented
Apr 12, 2025
nicolasnoble
commented
- proper cleaning cascading
- adding pushv for resolving ambiguity
- adding template to toUserdata
- adding LUA_MULTRET default to call / pcall
- refactored pcall to include a traceback
- properly using bit32 for fixedpoint instead of bit
- capturing newFromRaw locally for speedup
- opening Lua libraries on vm creation
- proper propagation of LUA_TARGET_PSX define
- proper cleaning cascading - adding pushv for resolving ambiguity - adding template to toUserdata - adding LUA_MULTRET default to call / pcall - refactored pcall to include a traceback - properly using bit32 for fixedpoint instead of bit - capturing newFromRaw locally for speedup - opening Lua libraries on vm creation - proper propagation of LUA_TARGET_PSX define
WalkthroughThis pull request updates several Makefiles and Lua integration components. The changes consolidate clean targets by switching to a double-colon syntax and removing redundant targets, and add the preprocessor flag Changes
Sequence Diagram(s)sequenceDiagram
participant Scene as C++ Scene
participant Lua as Lua Interpreter
participant Factorial as printfactorial Function
Scene->>Lua: Invoke printfactorial(6.0_fp)
Lua->>Factorial: Execute factorial calculation
alt Successful execution
Factorial-->>Lua: Return factorial result
else Error occurred
Lua->>Lua: Call traceback() for error handling
Factorial-->>Lua: Return error code
end
Lua-->>Scene: Return result (or error information)
Scene->>Scene: Print the result or handle error
Poem
Tip ⚡💬 Agentic Chat (Pro Plan, General Availability)
✨ Finishing Touches
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🔭 Outside diff range comments (1)
src/mips/psyqo-lua/src/lua.cpp (1)
166-369: 💡 Verification agent🧩 Analysis chain
Consider breaking down the setupFixedPointMetatable function
The static analysis tool has flagged the
setupFixedPointMetatablefunction for its high cyclomatic complexity. Consider breaking this function down into smaller, more manageable functions to improve code maintainability.You could separate the functionality into distinct methods such as:
setupFixedPointOperations- for setting up arithmetic operationssetupFixedPointComparisons- for setting up comparison operationssetupFixedPointConversions- for setting up conversion methods
🏁 Script executed:
#!/bin/bash # Check function complexity rg -A 5 "setupFixedPointMetatable" src/mips/psyqo-lua/src/lua.cppLength of output: 419
Action: Refactor the
setupFixedPointMetatableFunction to Lower ComplexityThe static analysis tool has flagged this function for its high cyclomatic complexity. Although the code functions as intended, its mixed responsibilities can make future maintenance and debugging challenging. Please consider refactoring by separating concerns into smaller helper functions. For example:
setupFixedPointOperations– Extract the C++ lambda implementations for arithmetic operations (multiplication, division, and__tostring).setupFixedPointComparisons/setupFixedPointConversions– Isolate functionality related to comparisons and conversions.loadFixedPointScript– Move the embedded Lua script loading and initialization into its own routine.This modular approach will not only reduce complexity but also improve overall code clarity and maintainability.
🧰 Tools
🪛 GitHub Check: CodeScene Cloud Delta Analysis (main)
[warning] 244-350: ❌ Getting worse: Complex Method
psyqo::Lua::setupFixedPointMetatable already has high cyclomatic complexity, and now it increases in Lines of Code from 158 to 159. This function has many conditional statements (e.g. if, for, while), leading to lower code health. Avoid adding more conditionals and code to it without refactoring.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
src/mips/common.mk(1 hunks)src/mips/psyqo-lua/Makefile(1 hunks)src/mips/psyqo-lua/examples/hello/hello.cpp(3 hunks)src/mips/psyqo-lua/lua.hh(5 hunks)src/mips/psyqo-lua/psyqo-lua.mk(2 hunks)src/mips/psyqo-lua/src/lua.cpp(10 hunks)src/mips/psyqo-paths/psyqo-paths.mk(1 hunks)src/mips/psyqo/psyqo.mk(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
src/mips/psyqo-lua/src/lua.cpp (1)
src/lua/luawrapper.h (1)
lua_gettop(179-179)
🪛 GitHub Check: CodeScene Cloud Delta Analysis (main)
src/mips/psyqo-lua/src/lua.cpp
[warning] 244-350: ❌ Getting worse: Complex Method
psyqo::Lua::setupFixedPointMetatable already has high cyclomatic complexity, and now it increases in Lines of Code from 158 to 159. This function has many conditional statements (e.g. if, for, while), leading to lower code health. Avoid adding more conditionals and code to it without refactoring.
[warning] 244-350: ❌ Getting worse: Complex Method
psyqo::Lua::setupFixedPointMetatable already has high cyclomatic complexity, and now it increases in Lines of Code from 158 to 159. This function has many conditional statements (e.g. if, for, while), leading to lower code health. Avoid adding more conditionals and code to it without refactoring.
⏰ Context from checks skipped due to timeout of 90000ms (11)
- GitHub Check: pcsx-redux (aarch64-linux)
- GitHub Check: pcsx-redux (x86_64-linux)
- GitHub Check: Codacy Static Code Analysis
- GitHub Check: build-openbios
- GitHub Check: aur-build
- GitHub Check: asan
- GitHub Check: toolchain
- GitHub Check: cross-arm64
- GitHub Check: build
- GitHub Check: macos-build-and-test-toolchain
- GitHub Check: coverage
🔇 Additional comments (24)
src/mips/psyqo-lua/Makefile (1)
10-10: Addition of LUA_TARGET_PSX define properly propagates the PSX target flagThis change adds the necessary compiler flag to indicate to the Lua code that it's targeting the PlayStation, which addresses one of the PR objectives. This enables conditional compilation for PSX-specific behaviors in the Lua implementation.
src/mips/common.mk (1)
111-111: Good implementation of proper cleaning cascadingChanging from single colon to double colon syntax for the
cleantarget allows for this target to be defined in multiple Makefiles, with all commands being executed whenmake cleanis run. This enables modular cleaning processes throughout the project hierarchy.src/mips/psyqo/psyqo.mk (1)
13-16: Good refactoring of the clean targetThe refactoring from what was likely a specific clean target (e.g.,
clean-psyqo:) to the double-colonclean::target aligns with the cascading clean approach implemented in common.mk. The .PHONY declaration is correctly updated to match this change.src/mips/psyqo-lua/examples/hello/hello.cpp (3)
48-52: New Lua function demonstrates improved functionalityThe
printfactorialfunction is a good addition that showcases how to format output in Lua and return values to the C++ caller. This supports the PR objectives by providing a practical example for using the enhanced Lua functionality.
168-168: Good addition of fixed-point literals namespaceIncluding the namespace for fixed-point literals enables the use of the
_fpsuffix in the code, making the fixed-point nature of values clear and explicit.
201-213: Excellent demonstration of the new fixed-point and error handling featuresThis code block effectively demonstrates:
- Getting a global Lua function
- Pushing a fixed-point value using the new functionality
- Calling the Lua function with proper error handling
- Converting the returned value back to a fixed-point type
- Properly cleaning up the stack with pop
This example aligns perfectly with the PR objectives regarding fixed-point operations, pcall refactoring, and pushing values.
src/mips/psyqo-paths/psyqo-paths.mk (1)
11-12: Updated clean target to use double colon syntaxThe clean target has been properly modified to use double colon syntax (
clean::). This allows for multiple definitions of the clean target across different makefiles, enabling proper cascading of clean operations.src/mips/psyqo-lua/psyqo-lua.mk (3)
7-7: Added LUA_TARGET_PSX defineGood addition of the
-DLUA_TARGET_PSXpreprocessor definition. This properly propagates the target platform information to the Lua build process.
22-24: Updated clean target to use double colon syntaxThe clean target has been properly modified to use double colon syntax (
clean::), allowing for multiple definitions of the clean target across different makefiles. This enables proper cascading of clean operations for both the current directory and the Lua directory.
26-26: Updated .PHONY declarationThe
.PHONYdeclaration has been correctly updated to includecleaninstead of the previous specific target.src/mips/psyqo-lua/src/lua.cpp (9)
29-29: Added EASTL string headerAdded
EASTL/string.hheader to support the use of EASTL strings in the new traceback function.
116-116: Opening Lua libraries in constructorGood improvement to open standard Lua libraries during VM creation, which makes the Lua environment more functional out of the box.
159-164: Renamed push to pushf for formatted stringsThe function
pushhas been renamed topushfto better indicate its purpose for formatted string pushing. This makes the API more explicit and resolves any ambiguity with other push methods.
244-248: Updated string formatting callsUpdated to use
pushfinstead ofpushfor string formatting. This change is consistent with the renamed API.🧰 Tools
🪛 GitHub Check: CodeScene Cloud Delta Analysis (main)
[warning] 244-350: ❌ Getting worse: Complex Method
psyqo::Lua::setupFixedPointMetatable already has high cyclomatic complexity, and now it increases in Lines of Code from 158 to 159. This function has many conditional statements (e.g. if, for, while), leading to lower code health. Avoid adding more conditionals and code to it without refactoring.
259-264: Added newFromRaw functionGood improvement to capture the
newFromRawfunction locally. This localizes the object creation logic and improves performance by avoiding repeated lookups of the metatable.
271-271: Replaced bit library with bit32Updated bit operations to use
bit32instead ofbit. This is the correct library to use for fixed-point operations in this context, as it provides a more standardized interface.Also applies to: 284-284, 303-303, 314-314, 325-325, 338-338
350-350: Improved loadBuffer with named chunkEnhanced the
loadBuffercall by adding a chunk name, which will make debugging easier by providing better context in error messages.🧰 Tools
🪛 GitHub Check: CodeScene Cloud Delta Analysis (main)
[warning] 244-350: ❌ Getting worse: Complex Method
psyqo::Lua::setupFixedPointMetatable already has high cyclomatic complexity, and now it increases in Lines of Code from 158 to 159. This function has many conditional statements (e.g. if, for, while), leading to lower code health. Avoid adding more conditionals and code to it without refactoring.
371-379: Implemented pcall with tracebackExcellent implementation of the
pcallmethod that incorporates a traceback mechanism. This will significantly improve error handling by providing more detailed stack traces when errors occur.
381-388: Added traceback functionWell-implemented traceback function that generates a comprehensive error report with stack trace. Using EASTL string for message handling is a good choice for memory management.
src/mips/psyqo-lua/lua.hh (5)
32-32: Added lualib.h includeGood addition of the
lualib.hheader, which is required for theluaL_openlibsfunction used in the constructor.
82-83: Added improved string formatting functionsGood addition of
vpushfandpushfmethods. This separates the implementation details from the interface and makes the API more explicit about formatting operations.
102-105: Made toUserdata templatedExcellent improvement to make
toUserdataa template function. This enhances type safety by allowing direct casting to the desired type, eliminating the need for additional manual casts at the call site.
149-150: Added LUA_MULTRET default valuesGood improvement to set
LUA_MULTRETas the default value fornresultsin bothcallandpcall. This simplifies usage by automatically handling the common case of returning all results.
205-205: Added traceback method declarationAdded the static
tracebackmethod declaration to support the enhanced error handling inpcall.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
.github/workflows/linux-toolchain.yml (1)
29-29: Enhance Robustness with Quoted Directory VariablesThe updated
runcommand now iterates over multiple directories, which is aligned with the PR objectives. However, for improved robustness in shell scripts, especially when directory paths might contain spaces, it's recommended to wrap the variable$din double quotes.- run: for d in src/mips/psyqo/examples/* src/mips/psyqo-paths/examples/* src/mips/psyqo-lua/examples/* ; do make -C $d -j 6 all TEST=true ; done + run: for d in src/mips/psyqo/examples/* src/mips/psyqo-paths/examples/* src/mips/psyqo-lua/examples/*; do make -C "$d" -j 6 all TEST=true; doneThis small change increases command reliability without affecting functionality.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
.github/workflows/linux-toolchain.yml(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (10)
- GitHub Check: pcsx-redux (aarch64-linux)
- GitHub Check: Codacy Static Code Analysis
- GitHub Check: build-openbios
- GitHub Check: asan
- GitHub Check: aur-build
- GitHub Check: coverage
- GitHub Check: cross-arm64
- GitHub Check: macos-build-and-test-toolchain
- GitHub Check: toolchain
- GitHub Check: build
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/mips/psyqo-lua/src/lua.cpp (1)
244-355: Consider refactoringsetupFixedPointMetatableto reduce complexity.Static analysis indicates this function has high cyclomatic complexity which is increasing with these changes. While the current changes are good, consider future refactoring to break this large function into smaller, more focused components.
You could extract parts of the metatable setup into separate helper functions, such as:
setupFixedPointArithmetic()setupFixedPointComparison()setupFixedPointConversion()This would make the code more maintainable and reduce the complexity of the main function.
🧰 Tools
🪛 GitHub Check: CodeScene Cloud Delta Analysis (main)
[warning] 244-355: ❌ Getting worse: Complex Method
psyqo::Lua::setupFixedPointMetatable already has high cyclomatic complexity, and now it increases in Lines of Code from 158 to 164. This function has many conditional statements (e.g. if, for, while), leading to lower code health. Avoid adding more conditionals and code to it without refactoring.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/mips/psyqo-lua/src/lua.cpp(7 hunks)third_party/psxlua(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- third_party/psxlua
🧰 Additional context used
🧬 Code Graph Analysis (1)
src/mips/psyqo-lua/src/lua.cpp (1)
src/lua/luawrapper.h (1)
lua_gettop(179-179)
🪛 GitHub Check: CodeScene Cloud Delta Analysis (main)
src/mips/psyqo-lua/src/lua.cpp
[warning] 244-355: ❌ Getting worse: Complex Method
psyqo::Lua::setupFixedPointMetatable already has high cyclomatic complexity, and now it increases in Lines of Code from 158 to 164. This function has many conditional statements (e.g. if, for, while), leading to lower code health. Avoid adding more conditionals and code to it without refactoring.
[warning] 244-355: ❌ Getting worse: Complex Method
psyqo::Lua::setupFixedPointMetatable already has high cyclomatic complexity, and now it increases in Lines of Code from 158 to 164. This function has many conditional statements (e.g. if, for, while), leading to lower code health. Avoid adding more conditionals and code to it without refactoring.
⏰ Context from checks skipped due to timeout of 90000ms (12)
- GitHub Check: macos-arm-build-and-test
- GitHub Check: macos-intel-build-and-test
- GitHub Check: pcsx-redux (aarch64-linux)
- GitHub Check: pcsx-redux (x86_64-linux)
- GitHub Check: Codacy Static Code Analysis
- GitHub Check: cross-arm64
- GitHub Check: build
- GitHub Check: asan
- GitHub Check: coverage
- GitHub Check: aur-build
- GitHub Check: macos-build-and-test-toolchain
- GitHub Check: toolchain
🔇 Additional comments (10)
src/mips/psyqo-lua/src/lua.cpp (10)
29-30: Nice addition of EASTL string header.The inclusion of EASTL's string header is appropriate and will be used by the new traceback functionality later in the file.
116-116: Great improvement to initialize standard Lua libraries automatically.Automatically opening standard Lua libraries in the constructor makes the Lua environment more complete and ready-to-use out of the box, aligning with the PR objective to open Lua libraries during VM creation.
159-164: Method renamed for clarity and to resolve ambiguity.Renaming
pushtopushffor formatted strings makes the API clearer. This aligns with the PR objective of adding apushvfunction to resolve ambiguity in the API.
244-248: Correctly updated to use the renamedpushfmethod.The code properly uses the renamed
pushfmethod for string formatting operations.🧰 Tools
🪛 GitHub Check: CodeScene Cloud Delta Analysis (main)
[warning] 244-355: ❌ Getting worse: Complex Method
psyqo::Lua::setupFixedPointMetatable already has high cyclomatic complexity, and now it increases in Lines of Code from 158 to 164. This function has many conditional statements (e.g. if, for, while), leading to lower code health. Avoid adding more conditionals and code to it without refactoring.
260-269: Good refactoring with local function capture and correct bit library usage.These changes implement two key PR objectives:
- Local capture of
newFromRawfor better performance- Correct usage of
bit32instead ofbitlibrary for fixed-point operationsThis refactoring encapsulates the FixedPoint creation logic in one place and uses the standard Lua 5.2 bit manipulation library.
274-309: Improved implementation of FixedPoint operations.The code correctly uses the new local functions for bit operations and the centralized
newFromRawfunction for FixedPoint creation, making the code more maintainable and efficient.
315-350: Consistent updates to remaining FixedPoint operations.All FixedPoint operations now consistently use the local bit functions and
newFromRaw, completing the refactoring throughout the metatable setup.
355-355: Enhanced debugging with named buffer.Adding a descriptive name for the buffer in the
loadBuffercall improves error reporting and debugging by providing context about the source of the code.🧰 Tools
🪛 GitHub Check: CodeScene Cloud Delta Analysis (main)
[warning] 244-355: ❌ Getting worse: Complex Method
psyqo::Lua::setupFixedPointMetatable already has high cyclomatic complexity, and now it increases in Lines of Code from 158 to 164. This function has many conditional statements (e.g. if, for, while), leading to lower code health. Avoid adding more conditionals and code to it without refactoring.
376-384: Excellent error handling enhancement with traceback integration.This new
pcallimplementation enhances error reporting by adding a traceback function as the error handler. The implementation properly manages the stack and aligns with the PR objective to refactorpcallto incorporate a traceback feature.
386-393: Well-implemented traceback function for better error diagnostics.The traceback function appropriately captures error messages and generates stack traces, making debugging much easier. It properly handles the case where no message is provided.
1 participant
