Adding CD authoring tool. #1906
Conversation
WalkthroughThe pull request consolidates updates across multiple build, source, and configuration files. The changes update the list of tools in Makefiles by adding Changes
Sequence Diagram(s)sequenceDiagram
participant U as User
participant AT as Authoring Tool
participant JP as JSON Parser
participant CW as Compression Worker
participant ISO as ISO Builder
U->>AT: Execute command with JSON input
AT->>JP: Parse and validate JSON
JP-->>AT: Return metadata
AT->>CW: Dispatch files for compression
CW-->>AT: Return compressed data
AT->>ISO: Build ISO image from sectors
ISO-->>AT: Finalize ISO construction
AT->>U: Output ISO image and status
Possibly related PRs
Poem
📜 Recent review detailsConfiguration used: .coderabbit.yaml 📒 Files selected for processing (1)
⏰ Context from checks skipped due to timeout of 90000ms (12)
🔇 Additional comments (2)
✨ 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: 2
🧹 Nitpick comments (9)
tools/exe2iso/exe2iso.cc (1)
90-198: Consider refactoring the main functionThe static analysis tool correctly points out that the
mainfunction has high cyclomatic complexity. Consider refactoring it into smaller, more focused functions to improve maintainability.You could extract the license handling logic (lines 168-198) into a separate function:
- if (!wroteLicense) { - memset(sector, 0, sizeof(sector)); - makeHeaderOnce(sector); - for (unsigned i = 0; i < 16; i++) { - writeSector(); - } - } + if (!wroteLicense) { + writeLicenseSectors(sector, writeSector); + } // Then define this function elsewhere: + void writeLicenseSectors(uint8_t sector[2352], const std::function<void()>& writeSector) { + memset(sector, 0, sizeof(sector)); + makeHeaderOnce(sector); + for (unsigned i = 0; i < 16; i++) { + writeSector(); + } + }🧰 Tools
🪛 GitHub Check: CodeScene Cloud Delta Analysis (main)
[warning] 98-194: ❌ Getting worse: Complex Method
main already has high cyclomatic complexity, and now it increases in Lines of Code from 131 to 132. 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.tools/authoring/README.md (3)
3-5: Improve grammar in requirements statementThe documentation clearly explains the purpose and functionality of the CD Authoring tool. However, there's a minor grammatical issue.
-The generated CD image will have its archive file stored at the sector 23, and as such, the archive manager may be initialized using this LBA. The filenames will be hashed using DJB2. Each entry may be compressed using the UCL-NRV2E algorithm. The LZ4 algorithm will not be used. This means the archive manager only requires to have the UCL-NRV2E decompressor registered. +The generated CD image will have its archive file stored at the sector 23, and as such, the archive manager may be initialized using this LBA. The filenames will be hashed using DJB2. Each entry may be compressed using the UCL-NRV2E algorithm. The LZ4 algorithm will not be used. This means the archive manager only requires having the UCL-NRV2E decompressor registered.🧰 Tools
🪛 LanguageTool
[grammar] ~5-~5: Did you mean “having”? Or maybe you should add a pronoun? In active voice, ‘require’ + ‘to’ takes an object, usually a pronoun.
Context: ...means the archive manager only requires to have the UCL-NRV2E decompressor registered. ...(ALLOW_TO)
34-34: Minor preposition usage issueThere's a small stylistic issue with preposition choice.
-All the file lookups will be relative to the location of the json file. The files will be stored into the archive in their order of appearance. +All the file lookups will be relative to the location of the json file. The files will be stored in the archive in their order of appearance.🧰 Tools
🪛 LanguageTool
[uncategorized] ~34-~34: The preposition “in” seems more likely in this position than the preposition “into”.
Context: ...the json file. The files will be stored into the archive in their order of appearanc...(AI_EN_LECTOR_REPLACEMENT_PREPOSITION_INTO_IN)
36-41: Improve command-line arguments formattingThe command-line arguments documentation is clear but has some formatting inconsistencies.
Consider using Markdown list formatting with proper spacing:
-The tool accepts the following command line arguments: - -- `-o`: The output file name for the CD image. Mandatory. -- `-license`: The license file to be used. Optional. Point to an existing valid PlayStation 1 disc, or an official license file. The tool will use the license file to generate the CD image. If not provided, the tool will generate an empty license on the CD image. -- `-threads`: The number of threads to use for compression. Optional. Default is the number of available CPU cores. +The tool accepts the following command line arguments: + +- `-o`: The output file name for the CD image. Mandatory. +- `-license`: The license file to be used. Optional. Point to an existing valid PlayStation 1 disc, or an official license file. The tool will use the license file to generate the CD image. If not provided, the tool will generate an empty license on the CD image. +- `-threads`: The number of threads to use for compression. Optional. Default is the number of available CPU cores.🧰 Tools
🪛 LanguageTool
[uncategorized] ~38-~38: Loose punctuation mark.
Context: ...ollowing command line arguments: --o: The output file name for the CD image. ...(UNLIKELY_OPENING_PUNCTUATION)
[uncategorized] ~39-~39: Loose punctuation mark.
Context: ...or the CD image. Mandatory. --license: The license file to be used. Optional. ...(UNLIKELY_OPENING_PUNCTUATION)
[uncategorized] ~40-~40: Loose punctuation mark.
Context: ...ty license on the CD image. --threads: The number of threads to use for compre...(UNLIKELY_OPENING_PUNCTUATION)
tools/authoring/authoring.cc (2)
124-125: Add exception handling for JSON parsing.
nlohmann::json::parse()can throw on malformed JSON. The current code only checksindexData.is_null()after parsing, which won't catch exceptions. Consider wrapping the parse call in atry/catchblock to handle JSON exceptions gracefully.try { auto indexData = nlohmann::json::parse(container.begin(), container.end()); +} catch (const nlohmann::json::parse_error& e) { + fmt::print("Error parsing JSON: {}\n", e.what()); + return -1; }
162-162: Handle case whenstd::thread::hardware_concurrency()returns 0.Some platforms may return 0, indicating that the number of hardware threads is not detectable. Defaulting to at least 1 thread can prevent potential runtime errors.
-const unsigned threadCount = args.get<unsigned>("threads", std::thread::hardware_concurrency()); +unsigned detectedThreads = std::thread::hardware_concurrency(); +if (detectedThreads == 0) { + detectedThreads = 1; +} +const unsigned threadCount = args.get<unsigned>("threads", detectedThreads);src/supportpsx/iso9660-builder.h (2)
36-36: Consider making the constructor explicit.To avoid unintended conversions, marking the constructor
ISO9660Builder(IO<File> out)asexplicitcan help clarify usage.-ISO9660Builder(IO<File> out) : m_out(out) {} +explicit ISO9660Builder(IO<File> out) : m_out(out) {}
44-47: Use RAII for resource cleanup.
close()must be called manually to finalize the output. Instead of manually invokingclose(), consider moving cleanup into a destructor or leveraging smart pointers, ensuring resources are always freed.~ISO9660Builder() { if (m_out && !m_out->failed()) { m_out->close(); } }vsprojects/pcsx-redux.sln (1)
584-584: Potentially conflicting solution GUID.There's a second
SolutionGuidwhich can overwrite or conflict with the earlier line (line 583). Consider removing one to avoid confusion in some Visual Studio versions.- SolutionGuid = {AC54A867-F976-4B3D-A6EF-F57EB764DCD4} +// Remove duplicate or merge if needed to avoid overshadowing
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (35)
.github/filter-support/Makefile-filtered(1 hunks).gitignore(1 hunks)Makefile(1 hunks)azure-pipelines-cli.yml(2 hunks)azure-pipelines.yml(2 hunks)src/cdrom/common.h(0 hunks)src/cdrom/file.cc(5 hunks)src/cdrom/file.h(3 hunks)src/cdrom/iso9660-builder.h(0 hunks)src/cdrom/iso9660-reader.cc(1 hunks)src/cdrom/iso9660-reader.h(1 hunks)src/core/luaiso.cc(3 hunks)src/core/web-server.cc(3 hunks)src/mips/common/util/util.h(1 hunks)src/support/binstruct.h(1 hunks)src/support/file.cc(2 hunks)src/support/file.h(3 hunks)src/supportpsx/binffi.lua(1 hunks)src/supportpsx/iec-60908b.h(1 hunks)src/supportpsx/iso9660-builder.cc(4 hunks)src/supportpsx/iso9660-builder.h(1 hunks)src/supportpsx/iso9660-lowlevel.h(3 hunks)tools/authoring/README.md(1 hunks)tools/authoring/authoring.cc(1 hunks)tools/exe2iso/exe2iso.cc(2 hunks)tools/modconv/modconv.cc(2 hunks)tools/ps1-packer/ps1-packer.cc(2 hunks)vsprojects/authoring/authoring.vcxproj(1 hunks)vsprojects/authoring/authoring.vcxproj.filters(1 hunks)vsprojects/cdrom/cdrom.vcxproj(0 hunks)vsprojects/cdrom/cdrom.vcxproj.filters(0 hunks)vsprojects/exe2iso/exe2iso.vcxproj(1 hunks)vsprojects/pcsx-redux.sln(3 hunks)vsprojects/supportpsx/supportpsx.vcxproj(2 hunks)vsprojects/supportpsx/supportpsx.vcxproj.filters(2 hunks)
💤 Files with no reviewable changes (4)
- src/cdrom/common.h
- src/cdrom/iso9660-builder.h
- vsprojects/cdrom/cdrom.vcxproj.filters
- vsprojects/cdrom/cdrom.vcxproj
🧰 Additional context used
🪛 GitHub Check: CodeScene Cloud Delta Analysis (main)
tools/exe2iso/exe2iso.cc
[warning] 98-194: ❌ Getting worse: Complex Method
main already has high cyclomatic complexity, and now it increases in Lines of Code from 131 to 132. 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] 98-194: ❌ Getting worse: Complex Method
main already has high cyclomatic complexity, and now it increases in Lines of Code from 131 to 132. 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.
tools/authoring/authoring.cc
[warning] 94-480: ❌ New issue: Complex Method
main has a cyclomatic complexity of 50, threshold = 9. 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] 103-103: ❌ New issue: Complex Conditional
main has 1 complex conditionals with 2 branches, threshold = 2. A complex conditional is an expression inside a branch (e.g. if, for, while) which consists of multiple, logical operators such as AND/OR. The more logical operators in an expression, the more severe the code smell.
[warning] 94-480: ❌ New issue: Bumpy Road Ahead
main has 5 blocks with nested conditional logic. Any nesting of 2 or deeper is considered. Threshold is one single, nested block per function. The Bumpy Road code smell is a function that contains multiple chunks of nested conditional logic. The deeper the nesting and the more bumps, the lower the code health.
[warning] 94-480: ❌ New issue: Deep, Nested Complexity
main has a nested complexity depth of 4, threshold = 4. This function contains deeply nested logic such as if statements and/or loops. The deeper the nesting, the lower the code health.
🪛 LanguageTool
tools/authoring/README.md
[grammar] ~5-~5: Did you mean “having”? Or maybe you should add a pronoun? In active voice, ‘require’ + ‘to’ takes an object, usually a pronoun.
Context: ...means the archive manager only requires to have the UCL-NRV2E decompressor registered. ...
(ALLOW_TO)
[uncategorized] ~34-~34: The preposition “in” seems more likely in this position than the preposition “into”.
Context: ...the json file. The files will be stored into the archive in their order of appearanc...
(AI_EN_LECTOR_REPLACEMENT_PREPOSITION_INTO_IN)
[uncategorized] ~38-~38: Loose punctuation mark.
Context: ...ollowing command line arguments: - -o: The output file name for the CD image. ...
(UNLIKELY_OPENING_PUNCTUATION)
[uncategorized] ~39-~39: Loose punctuation mark.
Context: ...or the CD image. Mandatory. - -license: The license file to be used. Optional. ...
(UNLIKELY_OPENING_PUNCTUATION)
[uncategorized] ~40-~40: Loose punctuation mark.
Context: ...ty license on the CD image. - -threads: The number of threads to use for compre...
(UNLIKELY_OPENING_PUNCTUATION)
⏰ Context from checks skipped due to timeout of 90000ms (12)
- GitHub Check: macos-intel-build-and-test
- GitHub Check: macos-arm-build-and-test
- GitHub Check: pcsx-redux (x86_64-linux)
- GitHub Check: pcsx-redux (aarch64-linux)
- GitHub Check: Codacy Static Code Analysis
- GitHub Check: toolchain
- GitHub Check: asan
- GitHub Check: aur-build
- GitHub Check: cross-arm64
- GitHub Check: macos-build-and-test-toolchain
- GitHub Check: build
- GitHub Check: coverage
🔇 Additional comments (61)
tools/modconv/modconv.cc (1)
58-58: Improved code safety with const qualifiersThe addition of
constqualifiers to variables likeoutput,inputs,samplesFile,amplification, andinputprevents accidental modification after initialization, improving code safety and readability.Also applies to: 66-67, 70-71, 90-90
.gitignore (1)
42-42: Added authoring tool to ignored binariesThe addition of
/authoringto the .gitignore file is consistent with how other tool binaries are handled in this project, ensuring that the compiled binary for the new CD authoring tool is properly excluded from version control.src/cdrom/iso9660-reader.h (1)
27-27:❓ Verification inconclusive
Updated include path for iso9660-lowlevel.h
The include path has been updated to reference
supportpsx/iso9660-lowlevel.hinstead ofcdrom/iso9660-lowlevel.h, reflecting the file's relocation as part of a broader refactoring effort.
🏁 Script executed:
#!/bin/bash # Verify that iso9660-lowlevel.h exists in the new location and check for any references to the old location echo "Checking for the header file in the new location:" find src -name "iso9660-lowlevel.h" echo -e "\nChecking for any remaining references to the old path:" rg "cdrom/iso9660-lowlevel.h" src --files-with-matchesLength of output: 358
Include Path Update Verified – Please Manually Confirm Overall Impact
- The header file now exists at
src/supportpsx/iso9660-lowlevel.has expected.- No occurrences of the old include path (
cdrom/iso9660-lowlevel.h) were found in the repository.The automated checks indicate that the file relocation is successful. However, since the grep command produced no output (which could be due to low-quality inference), please perform a manual verification to ensure no legacy references remain elsewhere in the codebase.
src/supportpsx/iec-60908b.h (1)
42-49:✅ Verification successful
Added SectorMode enum in IEC60908b namespace
This addition of the
SectorModeenum class properly encapsulates CD sector mode constants within theIEC60908bnamespace. The enum includes comprehensive documentation for each mode type with relevant size specifications, making the code more maintainable and self-documenting.This appears to be part of a larger refactoring effort to better organize CD-ROM related functionality by moving these definitions from
cdrom/common.hto this more appropriate location.
🏁 Script executed:
#!/bin/bash # Verify that references to SectorMode have been updated across the codebase echo "Checking for references to the old SectorMode enum:" rg "SectorMode\s+[^:]" src --type cpp echo -e "\nChecking for references to the new IEC60908b::SectorMode:" rg "IEC60908b::SectorMode" src --type cppLength of output: 3882
Verified Approval: The refactor is consistent.
The addition of the
SectorModeenum within theIEC60908bnamespace is correctly implemented and all references in the codebase now use the namespaced version. This change effectively moves the sector mode definitions fromcdrom/common.hto a more appropriate location, aligning with the broader CD-ROM functionality refactoring.
- The search results confirm that every reference is now fully qualified as
IEC60908b::SectorMode.- No deprecated or un-namespaced instances remain.
src/supportpsx/binffi.lua (1)
168-168:❓ Verification inconclusive
Buffer size increased by 16 bytes.
The buffer size calculation has been changed from
srcSize * 1.2 + 2048tosrcSize * 1.2 + 2064, adding 16 extra bytes to the buffer. This change likely accommodates additional memory requirements for the new CD authoring tool.
🏁 Script executed:
#!/bin/bash # Check if the increased buffer size is required by any new functionality in the authoring tool # Find all files that might use uclPack within the authoring tool implementation rg -A 5 -B 5 'uclPack|bufferSize' tools/authoring/Length of output: 52
Buffer size change requires manual confirmation
The current change insrc/supportpsx/binffi.lua(line 168) updates the buffer size calculation fromsrcSize * 1.2 + 2048tosrcSize * 1.2 + 2064(an increase of 16 bytes) to support the new CD authoring tool. However, our automated search in thetools/authoring/directory did not return any evidence linking this change to specific functionality (e.g., references touclPackor explicit buffer usage).
- Action item: Please manually verify that the 16-byte increase is indeed required by the new CD authoring tool’s implementation.
- Location to review:
src/supportpsx/binffi.lua(line 168)azure-pipelines.yml (2)
73-73: Adding authoring.exe to the build artifacts.The new CD authoring tool executable is now correctly included in the list of files to be copied to the binaries directory.
94-94: Excluding authoring.exe from test assemblies.The authoring tool is properly excluded from the test assemblies list, consistent with how other tools are handled.
Makefile (1)
175-175:✅ Verification successful
Adding authoring tool to the build system.
The new authoring tool has been added to the TOOLS variable, ensuring it will be compiled along with the other tools in the project.
🏁 Script executed:
#!/bin/bash # Verify that the authoring tool source files exist if [ -d "tools/authoring" ]; then echo "✅ Authoring tool directory exists" if [ -f "tools/authoring/authoring.cc" ]; then echo "✅ Main source file exists" else echo "❌ Missing main source file: tools/authoring/authoring.cc" fi else echo "❌ Missing authoring tool directory: tools/authoring" fiLength of output: 208
Verified Build System Update for Authoring Tool
The addition of the authoring tool to the TOOLS variable in the Makefile has been verified. The verification script confirmed that the "tools/authoring" directory exists and contains the main source file "tools/authoring/authoring.cc". The build system integration for the authoring tool is set up correctly.
azure-pipelines-cli.yml (2)
65-65: Adding authoring.exe to the CLI build artifacts.The authoring tool executable is correctly included in the list of files to be copied to the binaries directory for the CLI build configuration.
86-86: Excluding authoring.exe from CLI test assemblies.The authoring tool is properly excluded from the test assemblies list in the CLI build configuration, consistent with how other tools are handled.
.github/filter-support/Makefile-filtered (1)
88-88: LGTM! Tools list updated to include authoring toolThe update to the TOOLS variable correctly adds the new authoring tool, maintaining alphabetical order for better readability. This change supports the PR objective of adding the CD authoring tool.
src/mips/common/util/util.h (1)
57-64: Good addition of MIPS-specific unaligned memory access functionThe implementation correctly uses
__builtin_memcpyfor safe, unaligned memory access which is important on MIPS architecture. The function is properly guarded with#ifdef __mips__to ensure it's only available on the target architecture.This complements the existing C++ template functions in the Utilities namespace, but makes similar functionality available for C code as well.
vsprojects/exe2iso/exe2iso.vcxproj (1)
114-116:✅ Verification successful
Project dependency changed from cdrom to supportpsx
The project reference change aligns with the PR's goal of updating sector mode enums to use the IEC60908b namespace.
🏁 Script executed:
#!/bin/bash # Verify that exe2iso no longer depends on types or functions from the removed cdrom project # Look for any imports or uses of cdrom-related headers in exe2iso code echo "Checking for potential cdrom dependencies in exe2iso code:" rg -i "cdrom" "tools/exe2iso/" || echo "No direct references to cdrom found in exe2iso code"Length of output: 268
Dependency Update Verified: Shift from cdrom to supportpsx is Confirmed
The project reference change has been successfully verified. The search in the exe2iso codebase confirms that there are no lingering references to cdrom:
- File:
vsprojects/exe2iso/exe2iso.vcxproj(Lines 114-116) now correctly referencessupportpsx.vcxproj.- The removal of cdrom dependency is confirmed by the absence of any "cdrom" references in the
tools/exe2iso/folder.vsprojects/authoring/authoring.vcxproj.filters (1)
1-22: LGTM! New VS project filter file for the authoring toolThe file correctly follows standard Visual Studio project organization with appropriate filters for source, header, and resource files. The authoring.cc source file is properly linked to the Source Files filter.
src/cdrom/iso9660-reader.cc (1)
24-24: Updated include path to use supportpsx module.The include path has been updated from
cdrom/iso9660-lowlevel.htosupportpsx/iso9660-lowlevel.h, which aligns with the overall refactoring to move ISO9660 functionality into the supportpsx module. This change is consistent with the PR objective of adding CD authoring capabilities.vsprojects/supportpsx/supportpsx.vcxproj.filters (2)
51-59: Added IEC-60908b and ISO9660 builder source files.These additions integrate the necessary components for CD authoring, including EDC/ECC handling from the IEC-60908b standard and ISO9660 building functionality. The changes are well-structured and placed in the appropriate filter group.
119-130: Added ISO9660 and IEC-60908b header files.The header files complement the source files added earlier and provide the necessary interface definitions for ISO9660 filesystem building and IEC-60908b standard support. All files are appropriately categorized in the Header Files filter.
src/supportpsx/iso9660-lowlevel.h (2)
1-25: License changed from GPL to MIT.The license for this file has been changed from GPL to MIT, which is more permissive. This change allows for broader usage and integration of the ISO9660 low-level functionality in various contexts, supporting the CD authoring tool's flexibility.
34-34: Updated namespace declaration style.The namespace declaration has been modernized from nested syntax
namespace PCSX { namespace ISO9660LowLevel {to the more concise C++17 stylenamespace PCSX::ISO9660LowLevel {. This improves code readability while maintaining the same functionality.Also applies to: 136-136
vsprojects/authoring/authoring.vcxproj (1)
1-130: Added new Visual Studio project for CD authoring tool.This new project file correctly configures the CD authoring tool with appropriate build settings for different configurations (Debug, Release, ReleaseWithClangCL). The project dependencies are properly set up to reference fmt, supportpsx, support, and zlib, which are necessary components for the tool's functionality.
The project includes the main source file (authoring.cc) and sets up the correct include directories. The console subsystem and appropriate optimization settings are configured for each build type.
tools/exe2iso/exe2iso.cc (2)
98-99: Good practice: Using const for variables that shouldn't changeThe change from
autotoconst autofor these variables enforces immutability, which is a good practice for values that shouldn't be modified after initialization.🧰 Tools
🪛 GitHub Check: CodeScene Cloud Delta Analysis (main)
[warning] 98-194: ❌ Getting worse: Complex Method
main already has high cyclomatic complexity, and now it increases in Lines of Code from 131 to 132. 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.
194-194: Good addition: Ensuring header initializationAdding
makeHeaderOnce(sector)here ensures that the sector header is properly initialized before writing license sectors, which prevents potential issues with uninitialized header data.🧰 Tools
🪛 GitHub Check: CodeScene Cloud Delta Analysis (main)
[warning] 98-194: ❌ Getting worse: Complex Method
main already has high cyclomatic complexity, and now it increases in Lines of Code from 131 to 132. 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.tools/ps1-packer/ps1-packer.cc (2)
37-37: Good practice: Using const for immutable variablesThe change from
autotoconst autoforinputsenforces immutability, which is a good practice for values that shouldn't be modified after initialization.
50-50: Good practice: Using const for immutable variablesThe change from
unsignedtoconst unsignedforoutputTypeCountenforces immutability, which is a good practice for values that shouldn't be modified after initialization.src/support/file.h (3)
344-345: Good addition: New enum for memory borrowingThe new
Borrowenum provides a clear way to distinguish constructor behavior for memory handling, improving the API's clarity.
356-359: Good documentation: Clear explanation of borrowing behaviorThe added documentation clearly explains the behavior of the new constructor, highlighting that the buffer can be written to but won't expand if writes exceed its size.
377-377: Enhanced functionality: Added offset parameterAdding an optional
offsetparameter to theborrowmethod increases flexibility, allowing clients to create slices that start at specific positions within the buffer.src/support/file.cc (3)
55-59: Good implementation: Memory borrowing constructorThe new constructor properly initializes a
BufferFilethat references existing memory without taking ownership, setting the appropriate flags to ensure correct behavior.
155-155: Improved logic: Better duplication controlThe updated condition in the
dupmethod now considers both ownership and write status when determining how to duplicate a buffer, providing more appropriate behavior.
162-165: Enhanced implementation: Offset support in borrow methodThe
borrowmethod now properly handles the offset parameter, adjusting both the data pointer and remaining size to ensure the returned slice represents the correct subset of data.src/support/binstruct.h (2)
152-155: Good update to the assignment operator return type.The change to return
CString&instead ofCString<S>improves the class design by following the standard C++ pattern for assignment operators. This enables method chaining and is more efficient since it avoids unnecessary copying.
156-163: Enhanced string padding functionality with clear implementation.The updated
setmethod now takes an additional padding parameter with a sensible default value of0. The implementation correctly:
- Calculates the amount to copy using
std::min- Copies the input string data
- Pads the remaining space with the specified character
This enhancement improves the flexibility of the
CStringclass while maintaining backward compatibility.src/cdrom/file.h (3)
28-28: Updated include path for IEC-60908b standard.The change replaces an older header with the new
supportpsx/iec-60908b.hheader, which is part of the broader refactoring to organize CD-related code according to standards.
40-41: Updated constructor parameter type to use IEC60908b namespace.The constructor now properly uses the qualified type
IEC60908b::SectorModeinstead of the previously unqualifiedSectorMode. This change ensures consistency with the CD standard implementation across the codebase.
58-58: Updated member variable type to use IEC60908b namespace.The member variable
m_modenow uses the fully qualified typeIEC60908b::SectorModefor better clarity and organization.src/core/web-server.cc (3)
46-46: Updated include path for the ISO9660 builder.The include path has been updated to reference the builder from the new
supportpsxdirectory instead ofcdrom, which is part of the broader reorganization of CD-related functionality.
466-466: Updated SectorMode type to use IEC60908b namespace.The
modevariable declaration now properly uses the fully qualified typePCSX::IEC60908b::SectorModewith the appropriate default value.
476-477: Updated enum_cast to use the IEC60908b namespace.The
magic_enum::enum_castcall has been updated to use the fully qualified typePCSX::IEC60908b::SectorMode, maintaining consistency with the type changes throughout the codebase.src/supportpsx/iso9660-builder.cc (5)
1-25: License changed from GPL to MIT.The file's license has been updated from GNU General Public License to MIT License. This is a significant change that affects how the code can be used, distributed, and modified.
Verify with your team that this license change is intentional and aligns with the project's licensing strategy, as it changes the legal terms under which this code can be used.
27-27: Updated include path for the ISO9660 builder header.The include path has been updated to reference the header from the new
supportpsxdirectory, consistent with the reorganization of CD-related functionality.
42-43: Updated SectorMode references to use IEC60908b namespace.All references to the
SectorModeenum in thewriteLicensemethod have been updated to use the fully qualifiedIEC60908b::SectorModetype, maintaining consistency with the namespace changes throughout the codebase.Also applies to: 48-49, 57-58
61-62: Updated method signature to use IEC60908b namespace.The
writeSectorAtmethod signature has been updated to useIEC60908b::SectorModeinstead of justSectorMode, maintaining consistency with the namespace changes.
69-70: Updated case statements to use IEC60908b namespace.All case statements in the switch block have been updated to use the fully qualified
IEC60908b::SectorModeenum values, ensuring consistency with the namespace changes throughout the codebase.Also applies to: 72-73, 81-82, 95-96
src/cdrom/file.cc (5)
27-27: Function signature updated correctly to use the new namespaceThe constructor now accepts
IEC60908b::SectorModeinstead ofSectorMode, aligning with the standardized enumeration approach across the codebase.
34-36: Mode check updated to use the correct namespaceThe conditional checks have been properly updated to use the
IEC60908b::SectorModeenumeration.
51-52: Case values correctly migrated to the new namespaceAll mode values in the switch statement have been properly updated to use the
IEC60908b::SectorModenamespace.Also applies to: 60-63
77-77: Conditional check updated correctlyThe conditional check for mode types has been properly updated to use the
IEC60908b::SectorModenamespace.
203-204: Switch case values properly updatedThe case values in the switch statement have been correctly updated to use the
IEC60908b::SectorModenamespace.tools/authoring/README.md (2)
1-2: Clear and concise titleGood introduction to the CD Authoring tool with a clear title.
7-32: Well-structured JSON exampleThe JSON structure example is clear and comprehensive, accurately showing all available configuration options.
vsprojects/supportpsx/supportpsx.vcxproj (4)
150-150: Properly added new ISO9660 builder source fileThe addition of the ISO9660 builder source file fits well with the project's infrastructure.
153-154: Added IEC-60908b implementation filesThe addition of the EDC/ECC implementation files from the IEC-60908b standard will support the CD authoring functionality.
178-179: Added ISO9660 builder header filesThese header files complete the ISO9660 builder implementation, providing the necessary interfaces for the CD authoring tool.
182-183: Added IEC-60908b header filesThe header files for the EDC/ECC implementation properly complement the source files added earlier.
src/core/luaiso.cc (3)
30-30: Updated include path to use the new locationThe include path has been correctly updated to reference the ISO9660 builder implementation from the supportpsx directory instead of cdrom.
56-56: Updated function signature with new namespaceThe
fileisoOpenfunction signature has been properly updated to usePCSX::IEC60908b::SectorModeinstead ofSectorMode.
67-67: Updated function signature with new namespaceThe
isoBuilderWriteSectorfunction signature has been properly updated to usePCSX::IEC60908b::SectorModeinstead ofSectorMode.tools/authoring/authoring.cc (1)
252-323: Confirm concurrency approach for potential data races.Each thread works on a distinct
workUnitIndex, which seems safe, but all threads share the staticworkarray. The current approach is likely fine for smallfilesCount, but for complex concurrency scenarios, consider a thread-safe structure or container-based approach to ensure scalability and maintainability.Do you want to run a script to analyze usage of
workacross the codebase to confirm that only unique indices are accessed?vsprojects/pcsx-redux.sln (3)
112-113: Newauthoringproject added.Looks good. Properly references
"authoring\\authoring.vcxproj"with a unique GUID.
523-532: Configuration entries forauthoringproject.All build configurations (Debug, Release, etc.) match the existing pattern. The approach is consistent with other projects.
580-580: Nested project configuration forauthoring.Ensures
authoringis nested under tools. This is consistent with the existing solution layout.
tools/authoring/authoring.cc
Outdated
| PCSX::IO<PCSX::File> pvdSector(new PCSX::BufferFile(PCSX::FileOps::READWRITE)); | ||
| PCSX::ISO9660LowLevel::PVD pvd; | ||
| pvd.reset(); | ||
| pvd.get<PCSX::ISO9660LowLevel::PVD_TypeCode>().value = 1; | ||
| pvd.get<PCSX::ISO9660LowLevel::PVD_StdIdent>().set("CD001"); | ||
| pvd.get<PCSX::ISO9660LowLevel::PVD_Version>().value = 1; | ||
| auto systemIdent = pvdData["system_id"].is_string() ? pvdData["system_id"].get<std::string>() : "PLAYSTATION"; | ||
| pvd.get<PCSX::ISO9660LowLevel::PVD_SystemIdent>().set(systemIdent, ' '); | ||
| auto volumeIdent = pvdData["volume_id"].is_string() ? pvdData["volume_id"].get<std::string>() : ""; | ||
| pvd.get<PCSX::ISO9660LowLevel::PVD_VolumeIdent>().set(volumeIdent, ' '); | ||
| pvd.get<PCSX::ISO9660LowLevel::PVD_VolumeSpaceSize>().value = totalSectorCount; | ||
| pvd.get<PCSX::ISO9660LowLevel::PVD_VolumeSpaceSizeBE>().value = totalSectorCount; | ||
| pvd.get<PCSX::ISO9660LowLevel::PVD_VolumeSetSize>().value = 1; | ||
| pvd.get<PCSX::ISO9660LowLevel::PVD_VolumeSetSizeBE>().value = 1; | ||
| pvd.get<PCSX::ISO9660LowLevel::PVD_VolumeSequenceNumber>().value = 1; | ||
| pvd.get<PCSX::ISO9660LowLevel::PVD_VolumeSequenceNumberBE>().value = 1; | ||
| pvd.get<PCSX::ISO9660LowLevel::PVD_LogicalBlockSize>().value = 2048; | ||
| pvd.get<PCSX::ISO9660LowLevel::PVD_LogicalBlockSizeBE>().value = 2048; | ||
| pvd.get<PCSX::ISO9660LowLevel::PVD_PathTableSize>().value = 10; | ||
| pvd.get<PCSX::ISO9660LowLevel::PVD_PathTableSizeBE>().value = 10; | ||
| pvd.get<PCSX::ISO9660LowLevel::PVD_LPathTableLocation>().value = 18; | ||
| pvd.get<PCSX::ISO9660LowLevel::PVD_LPathTableOptLocation>().value = 19; | ||
| pvd.get<PCSX::ISO9660LowLevel::PVD_MPathTableLocation>().value = 20; | ||
| pvd.get<PCSX::ISO9660LowLevel::PVD_MPathTableOptLocation>().value = 21; | ||
| auto& root = pvd.get<PCSX::ISO9660LowLevel::PVD_RootDir>(); | ||
| root.get<PCSX::ISO9660LowLevel::DirEntry_Length>().value = 34; | ||
| root.get<PCSX::ISO9660LowLevel::DirEntry_ExtLength>().value = 0; | ||
| root.get<PCSX::ISO9660LowLevel::DirEntry_LBA>().value = 22; | ||
| root.get<PCSX::ISO9660LowLevel::DirEntry_LBABE>().value = 22; | ||
| root.get<PCSX::ISO9660LowLevel::DirEntry_Size>().value = 2048; | ||
| root.get<PCSX::ISO9660LowLevel::DirEntry_SizeBE>().value = 2048; | ||
| root.get<PCSX::ISO9660LowLevel::DirEntry_Flags>().value = 2; | ||
| root.get<PCSX::ISO9660LowLevel::DirEntry_VolSeqNo>().value = 1; | ||
| root.get<PCSX::ISO9660LowLevel::DirEntry_VolSeqNoBE>().value = 1; | ||
| root.get<PCSX::ISO9660LowLevel::DirEntry_Filename>().value.resize(1); | ||
| auto volumeSetIdent = pvdData["volume_set_id"].is_string() ? pvdData["volume_set_id"].get<std::string>() : ""; | ||
| pvd.get<PCSX::ISO9660LowLevel::PVD_VolSetIdent>().set(volumeSetIdent, ' '); | ||
| auto publisherIdent = pvdData["publisher"].is_string() ? pvdData["publisher"].get<std::string>() : ""; | ||
| pvd.get<PCSX::ISO9660LowLevel::PVD_PublisherIdent>().set(publisherIdent, ' '); | ||
| auto dataPreparerIdent = pvdData["preparer"].is_string() ? pvdData["preparer"].get<std::string>() : ""; | ||
| pvd.get<PCSX::ISO9660LowLevel::PVD_DataPreparerIdent>().set(dataPreparerIdent, ' '); | ||
| auto applicationIdent = pvdData["application_id"].is_string() ? pvdData["application_id"].get<std::string>() : ""; | ||
| pvd.get<PCSX::ISO9660LowLevel::PVD_ApplicationIdent>().set(applicationIdent, ' '); | ||
| auto copyrightFileIdent = pvdData["copyright"].is_string() ? pvdData["copyright"].get<std::string>() : ""; | ||
| pvd.get<PCSX::ISO9660LowLevel::PVD_CopyrightFileIdent>().set(copyrightFileIdent, ' '); | ||
| auto abstractFileIdent = pvdData["abstract"].is_string() ? pvdData["abstract"].get<std::string>() : ""; | ||
| pvd.get<PCSX::ISO9660LowLevel::PVD_AbstractFileIdent>().set(abstractFileIdent, ' '); | ||
| auto bibliographicFileIdent = | ||
| pvdData["bibliographic"].is_string() ? pvdData["bibliographic"].get<std::string>() : ""; | ||
| pvd.get<PCSX::ISO9660LowLevel::PVD_BibliographicFileIdent>().set(bibliographicFileIdent, ' '); | ||
| pvd.get<PCSX::ISO9660LowLevel::PVD_FileStructureVersion>().value = 1; | ||
|
|
||
| pvd.serialize(pvdSector); | ||
| while (pvdSector->size() < 2048) { | ||
| pvdSector->write<uint8_t>(0); | ||
| } | ||
| builder.writeSectorAt(pvdSector.asA<PCSX::BufferFile>()->borrow(0).data<uint8_t>(), {0, 2, 16}, | ||
| PCSX::IEC60908b::SectorMode::M2_FORM1); | ||
|
|
||
| uint8_t sector[2048]; | ||
| memset(sector, 0, sizeof(sector)); | ||
| sector[0] = 0xff; | ||
| sector[1] = 'C'; | ||
| sector[2] = 'D'; | ||
| sector[3] = '0'; | ||
| sector[4] = '0'; | ||
| sector[5] = '1'; | ||
| builder.writeSectorAt(sector, {0, 2, 17}, PCSX::IEC60908b::SectorMode::M2_FORM1); | ||
|
|
||
| memset(sector, 0, sizeof(sector)); | ||
| sector[0] = 1; | ||
| sector[2] = 22; | ||
| sector[6] = 1; | ||
| builder.writeSectorAt(sector, {0, 2, 18}, PCSX::IEC60908b::SectorMode::M2_FORM1); | ||
| builder.writeSectorAt(sector, {0, 2, 19}, PCSX::IEC60908b::SectorMode::M2_FORM1); | ||
|
|
||
| memset(sector, 0, sizeof(sector)); | ||
| sector[0] = 1; | ||
| sector[5] = 22; | ||
| sector[7] = 1; | ||
| builder.writeSectorAt(sector, {0, 2, 20}, PCSX::IEC60908b::SectorMode::M2_FORM1); | ||
| builder.writeSectorAt(sector, {0, 2, 21}, PCSX::IEC60908b::SectorMode::M2_FORM1); | ||
|
|
||
| uint8_t rootSector[2048] = { | ||
| 0x22, 0x00, 0x16, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x16, 0x6e, 0x00, 0x00, 0x00, 0x00, 0x00, | ||
| 0x00, 0x6e, 0x7d, 0x03, 0x10, 0x09, 0x1c, 0x27, 0xe4, 0x02, 0x00, 0x00, 0x01, 0x00, 0x00, 0x01, | ||
| 0x01, 0x00, 0x22, 0x00, 0x16, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x16, 0x6e, 0x00, 0x00, 0x00, | ||
| 0x00, 0x00, 0x00, 0x6e, 0x7d, 0x03, 0x10, 0x09, 0x1c, 0x27, 0xe4, 0x02, 0x00, 0x00, 0x01, 0x00, | ||
| 0x00, 0x01, 0x01, 0x01, 0x2a, 0x00, 0x57, 0x02, 0x00, 0x00, 0x00, 0x00, 0x02, 0x57, 0x21, 0x00, | ||
| 0x00, 0x00, 0x00, 0x00, 0x00, 0x21, 0x7d, 0x03, 0x10, 0x09, 0x1b, 0x36, 0xe4, 0x00, 0x00, 0x00, | ||
| 0x01, 0x00, 0x00, 0x01, 0x09, 0x50, 0x53, 0x58, 0x2e, 0x45, 0x58, 0x45, 0x3b, 0x31, | ||
| }; | ||
| writeToBuffer<uint32_t, std::endian::little>(rootSector + 70, indexSectorsCount + 23); | ||
| writeToBuffer<uint32_t, std::endian::big>(rootSector + 74, indexSectorsCount + 23); | ||
| writeToBuffer<uint32_t, std::endian::little>(rootSector + 78, executableSectorsCount * 2048); | ||
| writeToBuffer<uint32_t, std::endian::big>(rootSector + 82, executableSectorsCount * 2048); | ||
| builder.writeSectorAt(rootSector, {0, 2, 22}, PCSX::IEC60908b::SectorMode::M2_FORM1); | ||
|
|
||
| return 0; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Refactor main() to reduce complexity.
This function has grown quite large and has a cyclomatic complexity of 50. Extracting logical sections (e.g., argument-parsing, file-loading, compression, and sector-writing) into helper functions or separate classes would improve readability, maintainability, and testability.
🧰 Tools
🪛 GitHub Check: CodeScene Cloud Delta Analysis (main)
[warning] 94-480: ❌ New issue: Complex Method
main has a cyclomatic complexity of 50, threshold = 9. 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] 103-103: ❌ New issue: Complex Conditional
main has 1 complex conditionals with 2 branches, threshold = 2. A complex conditional is an expression inside a branch (e.g. if, for, while) which consists of multiple, logical operators such as AND/OR. The more logical operators in an expression, the more severe the code smell.
[warning] 94-480: ❌ New issue: Bumpy Road Ahead
main has 5 blocks with nested conditional logic. Any nesting of 2 or deeper is considered. Threshold is one single, nested block per function. The Bumpy Road code smell is a function that contains multiple chunks of nested conditional logic. The deeper the nesting and the more bumps, the lower the code health.
[warning] 94-480: ❌ New issue: Deep, Nested Complexity
main has a nested complexity depth of 4, threshold = 4. This function contains deeply nested logic such as if statements and/or loops. The deeper the nesting, the lower the code health.
nicolasnoble
force-pushed
the
authoring
branch
3 times, most recently
from
dbb7768 to
30a65da
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
tools/authoring/authoring.cc (1)
93-490: 🛠️ Refactor suggestionRefactor main() to reduce complexity.
The main function has high cyclomatic complexity (52) with multiple nested conditionals. Consider extracting logical sections into helper functions.
The function could be broken down into several smaller functions:
parseCommandLineArgs()loadAndValidateJsonInput()processExecutable()processFilesInParallel()writeIndexAndMetadata()This would improve readability, maintainability, and testability while keeping the main function as a high-level orchestrator.
🧰 Tools
🪛 GitHub Check: CodeScene Cloud Delta Analysis (main)
[warning] 93-490: ❌ New issue: Complex Method
main has a cyclomatic complexity of 52, threshold = 9. 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] 102-102: ❌ New issue: Complex Conditional
main has 1 complex conditionals with 2 branches, threshold = 2. A complex conditional is an expression inside a branch (e.g. if, for, while) which consists of multiple, logical operators such as AND/OR. The more logical operators in an expression, the more severe the code smell.
[warning] 93-490: ❌ New issue: Bumpy Road Ahead
main has 5 blocks with nested conditional logic. Any nesting of 2 or deeper is considered. Threshold is one single, nested block per function. The Bumpy Road code smell is a function that contains multiple chunks of nested conditional logic. The deeper the nesting and the more bumps, the lower the code health.
[warning] 93-490: ❌ New issue: Deep, Nested Complexity
main has a nested complexity depth of 4, threshold = 4. This function contains deeply nested logic such as if statements and/or loops. The deeper the nesting, the lower the code health.
🧹 Nitpick comments (8)
tools/exe2iso/exe2iso.cc (1)
98-236: Consider refactoring main function to reduce complexity.The static analysis tools have flagged that the
mainfunction has high cyclomatic complexity. Consider breaking it down into smaller, focused functions to improve maintainability.For example, you could extract functionality into separate methods like:
processLicenseFile()writeIsoStructure()writeExeData()writeAdditionalData()This would make the code more readable and easier to maintain.
🧰 Tools
🪛 GitHub Check: CodeScene Cloud Delta Analysis (main)
[warning] 98-194: ❌ Getting worse: Complex Method
main already has high cyclomatic complexity, and now it increases in Lines of Code from 131 to 132. 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.tools/authoring/README.md (3)
5-5: Minor grammatical improvement needed.The phrase "requires to have" is awkwardly phrased.
-The generated CD image will have its archive file stored at the sector 23, and as such, the archive manager may be initialized using this LBA. The filenames will be hashed using DJB2. Each entry may be compressed using the UCL-NRV2E algorithm. The LZ4 algorithm will not be used. This means the archive manager only requires to have the UCL-NRV2E decompressor registered. +The generated CD image will have its archive file stored at the sector 23, and as such, the archive manager may be initialized using this LBA. The filenames will be hashed using DJB2. Each entry may be compressed using the UCL-NRV2E algorithm. The LZ4 algorithm will not be used. This means the archive manager only requires the UCL-NRV2E decompressor to be registered.🧰 Tools
🪛 LanguageTool
[grammar] ~5-~5: Did you mean “having”? Or maybe you should add a pronoun? In active voice, ‘require’ + ‘to’ takes an object, usually a pronoun.
Context: ...means the archive manager only requires to have the UCL-NRV2E decompressor registered. ...(ALLOW_TO)
35-35: Minor grammatical corrections needed.There are a couple of grammatical issues in this sentence.
-All the file lookups will be relative to base directory, which is the same as the json file if the option `-basedir` isn't specified. The files will be stored into the archive in their order of appearance. +All the file lookups will be relative to the base directory, which is the same as the json file if the option `-basedir` isn't specified. The files will be stored in the archive in their order of appearance.🧰 Tools
🪛 LanguageTool
[uncategorized] ~35-~35: You might be missing the article “the” here.
Context: ...ll the file lookups will be relative to base directory, which is the same as the jso...(AI_EN_LECTOR_MISSING_DETERMINER_THE)
[uncategorized] ~35-~35: The preposition “in” seems more likely in this position than the preposition “into”.
Context: ...n't specified. The files will be stored into the archive in their order of appearanc...(AI_EN_LECTOR_REPLACEMENT_PREPOSITION_INTO_IN)
39-42: Improve bullet point formatting.The command-line arguments section would be more readable with proper Markdown list formatting.
-The tool accepts the following command line arguments: - -- `-o`: The output file name for the CD image. Mandatory. -- `-license`: The license file to be used. Optional. Point to an existing valid PlayStation 1 disc, or an official license file. The tool will use the license file to generate the CD image. If not provided, the tool will generate an empty license on the CD image. -- `-basedir`: The base directory for the files. Optional. Default is the directory where the json file is located. -- `-threads`: The number of threads to use for compression. Optional. Default is the number of available CPU cores. +The tool accepts the following command line arguments: + +- `-o`: The output file name for the CD image. Mandatory. +- `-license`: The license file to be used. Optional. Point to an existing valid PlayStation 1 disc, or an official license file. The tool will use the license file to generate the CD image. If not provided, the tool will generate an empty license on the CD image. +- `-basedir`: The base directory for the files. Optional. Default is the directory where the json file is located. +- `-threads`: The number of threads to use for compression. Optional. Default is the number of available CPU cores.🧰 Tools
🪛 LanguageTool
[uncategorized] ~39-~39: Loose punctuation mark.
Context: ...ollowing command line arguments: --o: The output file name for the CD image. ...(UNLIKELY_OPENING_PUNCTUATION)
[uncategorized] ~40-~40: Loose punctuation mark.
Context: ...or the CD image. Mandatory. --license: The license file to be used. Optional. ...(UNLIKELY_OPENING_PUNCTUATION)
[uncategorized] ~41-~41: Loose punctuation mark.
Context: ...ty license on the CD image. --basedir: The base directory for the files. Optio...(UNLIKELY_OPENING_PUNCTUATION)
[uncategorized] ~42-~42: Loose punctuation mark.
Context: ...e the json file is located. --threads: The number of threads to use for compre...(UNLIKELY_OPENING_PUNCTUATION)
tools/authoring/authoring.cc (3)
58-58: Consider documenting the maximum sector count constant.It would be helpful to add a comment explaining the derivation of this value (appears to be related to CD capacity).
-static constexpr unsigned c_maximumSectorCount = (99 * 60 + 59) * 75 + 74 - 150; +// Maximum sector count for a standard CD (99 minutes, 59 seconds, 74 frames) minus the 150-sector lead-in +static constexpr unsigned c_maximumSectorCount = (99 * 60 + 59) * 75 + 74 - 150;
259-333: Thread management could be improved.The code detaches threads but doesn't have a clean shutdown mechanism. This could lead to resource leaks if the program terminates abnormally.
Consider using a thread pool pattern or std::jthread (C++20) for better thread lifecycle management:
-for (unsigned i = 0; i < threadCount; i++) { - std::thread t([&]() { +std::vector<std::jthread> threads; +threads.reserve(threadCount); +for (unsigned i = 0; i < threadCount; i++) { + threads.emplace_back([&]() { // thread work... - }); - t.detach(); + }); } +// threads will automatically join when they go out of scope
291-291: Missing explicit size check for decompression safety.When compressing data, there's no explicit verification that the compressed data fits within the allocated buffer. Although
outSizeshould indicate the actual size written, a safety check before accessing memory would be prudent.r = ucl_nrv2e_99_compress(dataIn.data(), size, dataOut.data() + 2048, &outSize, nullptr, 10, nullptr, nullptr); if (r != UCL_E_OK) { workUnit.failed = true; workUnit.semaphore.release(); continue; } +if (outSize > dataOut.size() - 2048) { + fmt::print("Compression error: output buffer overflow\n"); + workUnit.failed = true; + workUnit.semaphore.release(); + continue; +}src/supportpsx/iso9660-builder.h (1)
40-43: Consider adding documentation for public methods.While the code is readable, adding documentation comments would improve understandability, especially for the parameters and return values.
+ /** + * @brief Writes a sector of data to the output at the current location + * @param sectorData Pointer to the data to be written + * @param mode The mode in which to write the sector + * @return The MSF (minutes/seconds/frames) location of the written sector + */ IEC60908b::MSF writeSector(const uint8_t* sectorData, IEC60908b::SectorMode mode) { return writeSectorAt(sectorData, m_location++, mode); }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (35)
.github/filter-support/Makefile-filtered(1 hunks).gitignore(1 hunks)Makefile(1 hunks)azure-pipelines-cli.yml(2 hunks)azure-pipelines.yml(2 hunks)src/cdrom/common.h(0 hunks)src/cdrom/file.cc(5 hunks)src/cdrom/file.h(3 hunks)src/cdrom/iso9660-builder.h(0 hunks)src/cdrom/iso9660-reader.cc(1 hunks)src/cdrom/iso9660-reader.h(1 hunks)src/core/luaiso.cc(3 hunks)src/core/web-server.cc(3 hunks)src/mips/common/util/util.h(1 hunks)src/support/binstruct.h(1 hunks)src/support/file.cc(2 hunks)src/support/file.h(3 hunks)src/supportpsx/binffi.lua(1 hunks)src/supportpsx/iec-60908b.h(1 hunks)src/supportpsx/iso9660-builder.cc(4 hunks)src/supportpsx/iso9660-builder.h(1 hunks)src/supportpsx/iso9660-lowlevel.h(3 hunks)tools/authoring/README.md(1 hunks)tools/authoring/authoring.cc(1 hunks)tools/exe2iso/exe2iso.cc(2 hunks)tools/modconv/modconv.cc(2 hunks)tools/ps1-packer/ps1-packer.cc(2 hunks)vsprojects/authoring/authoring.vcxproj(1 hunks)vsprojects/authoring/authoring.vcxproj.filters(1 hunks)vsprojects/cdrom/cdrom.vcxproj(0 hunks)vsprojects/cdrom/cdrom.vcxproj.filters(0 hunks)vsprojects/exe2iso/exe2iso.vcxproj(1 hunks)vsprojects/pcsx-redux.sln(3 hunks)vsprojects/supportpsx/supportpsx.vcxproj(2 hunks)vsprojects/supportpsx/supportpsx.vcxproj.filters(2 hunks)
💤 Files with no reviewable changes (4)
- src/cdrom/common.h
- src/cdrom/iso9660-builder.h
- vsprojects/cdrom/cdrom.vcxproj
- vsprojects/cdrom/cdrom.vcxproj.filters
🚧 Files skipped from review as they are similar to previous changes (21)
- src/cdrom/iso9660-reader.h
- src/supportpsx/binffi.lua
- .github/filter-support/Makefile-filtered
- azure-pipelines-cli.yml
- azure-pipelines.yml
- src/cdrom/iso9660-reader.cc
- vsprojects/exe2iso/exe2iso.vcxproj
- src/supportpsx/iec-60908b.h
- .gitignore
- vsprojects/authoring/authoring.vcxproj.filters
- src/support/binstruct.h
- src/cdrom/file.h
- src/supportpsx/iso9660-lowlevel.h
- src/cdrom/file.cc
- src/core/luaiso.cc
- tools/ps1-packer/ps1-packer.cc
- vsprojects/supportpsx/supportpsx.vcxproj.filters
- vsprojects/supportpsx/supportpsx.vcxproj
- Makefile
- src/core/web-server.cc
- tools/modconv/modconv.cc
🧰 Additional context used
🪛 GitHub Check: CodeScene Cloud Delta Analysis (main)
tools/authoring/authoring.cc
[warning] 93-490: ❌ New issue: Complex Method
main has a cyclomatic complexity of 52, threshold = 9. 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] 102-102: ❌ New issue: Complex Conditional
main has 1 complex conditionals with 2 branches, threshold = 2. A complex conditional is an expression inside a branch (e.g. if, for, while) which consists of multiple, logical operators such as AND/OR. The more logical operators in an expression, the more severe the code smell.
[warning] 93-490: ❌ New issue: Bumpy Road Ahead
main has 5 blocks with nested conditional logic. Any nesting of 2 or deeper is considered. Threshold is one single, nested block per function. The Bumpy Road code smell is a function that contains multiple chunks of nested conditional logic. The deeper the nesting and the more bumps, the lower the code health.
[warning] 93-490: ❌ New issue: Deep, Nested Complexity
main has a nested complexity depth of 4, threshold = 4. This function contains deeply nested logic such as if statements and/or loops. The deeper the nesting, the lower the code health.
tools/exe2iso/exe2iso.cc
[warning] 98-194: ❌ Getting worse: Complex Method
main already has high cyclomatic complexity, and now it increases in Lines of Code from 131 to 132. 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] 98-194: ❌ Getting worse: Complex Method
main already has high cyclomatic complexity, and now it increases in Lines of Code from 131 to 132. 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.
🪛 LanguageTool
tools/authoring/README.md
[grammar] ~5-~5: Did you mean “having”? Or maybe you should add a pronoun? In active voice, ‘require’ + ‘to’ takes an object, usually a pronoun.
Context: ...means the archive manager only requires to have the UCL-NRV2E decompressor registered. ...
(ALLOW_TO)
[uncategorized] ~35-~35: You might be missing the article “the” here.
Context: ...ll the file lookups will be relative to base directory, which is the same as the jso...
(AI_EN_LECTOR_MISSING_DETERMINER_THE)
[uncategorized] ~35-~35: The preposition “in” seems more likely in this position than the preposition “into”.
Context: ...n't specified. The files will be stored into the archive in their order of appearanc...
(AI_EN_LECTOR_REPLACEMENT_PREPOSITION_INTO_IN)
[uncategorized] ~39-~39: Loose punctuation mark.
Context: ...ollowing command line arguments: - -o: The output file name for the CD image. ...
(UNLIKELY_OPENING_PUNCTUATION)
[uncategorized] ~40-~40: Loose punctuation mark.
Context: ...or the CD image. Mandatory. - -license: The license file to be used. Optional. ...
(UNLIKELY_OPENING_PUNCTUATION)
[uncategorized] ~41-~41: Loose punctuation mark.
Context: ...ty license on the CD image. - -basedir: The base directory for the files. Optio...
(UNLIKELY_OPENING_PUNCTUATION)
[uncategorized] ~42-~42: Loose punctuation mark.
Context: ...e the json file is located. - -threads: The number of threads to use for compre...
(UNLIKELY_OPENING_PUNCTUATION)
⏰ Context from checks skipped due to timeout of 90000ms (11)
- GitHub Check: pcsx-redux (x86_64-linux)
- GitHub Check: pcsx-redux (aarch64-linux)
- GitHub Check: Codacy Static Code Analysis
- GitHub Check: cross-arm64
- GitHub Check: toolchain
- GitHub Check: asan
- GitHub Check: coverage
- GitHub Check: macos-build-and-test-toolchain
- GitHub Check: aur-build
- GitHub Check: build-openbios
- GitHub Check: build
🔇 Additional comments (23)
vsprojects/authoring/authoring.vcxproj (1)
1-130: Project configuration looks good!The new Visual Studio project for the CD authoring tool is properly configured with appropriate build settings for Debug, Release, and ReleaseWithClangCL configurations. All necessary references to dependent projects (fmt, supportpsx, support, zlib) are included, and the build settings follow the same patterns as other tools in the solution.
vsprojects/pcsx-redux.sln (4)
112-113: Project declaration properly added to the solution.The authoring project is correctly declared with its unique GUID and file path.
523-532: Build configurations properly defined for all solution configurations.All necessary build configurations (Debug, Release, ReleaseCLI, ReleaseWithClangCL, ReleaseWithTracy) have been properly configured for the new authoring project, following the same pattern as other tools in the solution.
580-580: Project correctly placed in the tools folder.The new authoring project is correctly organized under the tools solution folder, which is appropriate for its functionality.
584-584: Solution GUID updated appropriately.The SolutionGuid has been updated, which is expected when making changes to the solution structure.
src/mips/common/util/util.h (1)
57-64: Well-implemented utility for unaligned memory access on MIPS.This is a good addition for the MIPS architecture which traditionally has issues with unaligned memory access. The function properly uses
__builtin_memcpyto safely handle potential alignment issues, which is the recommended approach.src/support/file.h (3)
344-344: New enum to support borrowed memory semantics.Adding the
Borrowenum provides a clear way to differentiate constructor behavior for memory management.
356-359: Well-documented constructor behavior.The documentation clearly explains that while the buffer can be written to, it won't be expanded - an important distinction from other constructors.
377-377: Enhanced flexibility with backward compatibility.Adding a default parameter to
borrow()maintains compatibility with existing code while adding the ability to create slices from specific offsets.tools/exe2iso/exe2iso.cc (2)
98-99: Improved immutability guarantees.Changing from
autotoconst autofor these variables ensures they cannot be accidentally modified, making the code more robust.🧰 Tools
🪛 GitHub Check: CodeScene Cloud Delta Analysis (main)
[warning] 98-194: ❌ Getting worse: Complex Method
main already has high cyclomatic complexity, and now it increases in Lines of Code from 131 to 132. 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.
194-194: Fixed potential issue with header initialization.Adding the call to
makeHeaderOnce(sector)ensures the sector header is properly initialized even when no license is written.🧰 Tools
🪛 GitHub Check: CodeScene Cloud Delta Analysis (main)
[warning] 98-194: ❌ Getting worse: Complex Method
main already has high cyclomatic complexity, and now it increases in Lines of Code from 131 to 132. 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.src/support/file.cc (3)
55-59: Correct implementation of borrowed memory constructor.The constructor properly initializes the
BufferFilewith the provided data pointer and size, settingm_ownedto false to indicate that the memory is borrowed rather than owned.
155-155: Enhanced ownership handling in duplication.The modification to check both ownership and writability status before deciding how to duplicate is a logical improvement.
162-165: Properly implemented offset parameter for borrow method.The implementation correctly adds the offset to the data pointer and adjusts the size accordingly.
src/supportpsx/iso9660-builder.cc (3)
1-25: License updated to MIT.The file's license has been changed from GNU General Public License to MIT License, which is more permissive and aligns with the project's licensing strategy.
27-27: Added inclusion of header file.The implementation file now properly includes its corresponding header file.
42-43: Namespace qualified SectorMode references.All references to
SectorModeenum have been updated to use the fully qualified nameIEC60908b::SectorMode. This improves code clarity and avoids potential namespace conflicts.Also applies to: 48-50, 61-62, 69-70, 72-73, 81-82, 95-96
tools/authoring/README.md (1)
1-5: Documentation for the new CD Authoring tool looks good.The introduction and explanation of the tool's purpose and functionality are clear and informative.
🧰 Tools
🪛 LanguageTool
[grammar] ~5-~5: Did you mean “having”? Or maybe you should add a pronoun? In active voice, ‘require’ + ‘to’ takes an object, usually a pronoun.
Context: ...means the archive manager only requires to have the UCL-NRV2E decompressor registered. ...(ALLOW_TO)
tools/authoring/authoring.cc (3)
46-56: Well-implemented templated buffer writing utility.The
writeToBufferfunction handles endianness properly and utilizes a concept constraint for type safety.
60-89: Well-structured IndexEntry union with clear bit field definitions.The IndexEntry union is well designed with appropriate bit field spans and accessor methods for each field.
232-251: Added protection against file count overflow.Good addition of a check to prevent overflow when the number of files exceeds the maximum sector count.
src/supportpsx/iso9660-builder.h (2)
1-25: License header uses MIT license.The file uses the MIT license, consistent with the implementation file.
34-52: Well-designed class structure for ISO9660Builder.The class has a clear interface with appropriate methods for building ISO 9660 file systems. Good use of namespaces, encapsulation, and method signatures.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
tools/authoring/authoring.cc (1)
93-493: 🛠️ Refactor suggestionRefactor main() to reduce complexity.
This function has a cyclomatic complexity of 53 with multiple nested conditionals and distinct logical sections. Consider breaking it down into smaller, focused functions such as:
parseArguments()loadAndValidateJson()processExecutable()compressFiles()writeSectors()These smaller functions would improve readability, maintainability, and testability.
🧰 Tools
🪛 GitHub Check: CodeScene Cloud Delta Analysis (main)
[warning] 93-493: ❌ New issue: Complex Method
main has a cyclomatic complexity of 53, threshold = 9. 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] 102-102: ❌ New issue: Complex Conditional
main has 1 complex conditionals with 2 branches, threshold = 2. A complex conditional is an expression inside a branch (e.g. if, for, while) which consists of multiple, logical operators such as AND/OR. The more logical operators in an expression, the more severe the code smell.
[warning] 93-493: ❌ New issue: Bumpy Road Ahead
main has 5 blocks with nested conditional logic. Any nesting of 2 or deeper is considered. Threshold is one single, nested block per function. The Bumpy Road code smell is a function that contains multiple chunks of nested conditional logic. The deeper the nesting and the more bumps, the lower the code health.
[warning] 93-493: ❌ New issue: Deep, Nested Complexity
main has a nested complexity depth of 4, threshold = 4. This function contains deeply nested logic such as if statements and/or loops. The deeper the nesting, the lower the code health.
🧹 Nitpick comments (2)
src/support/file.cc (1)
162-165: Added bounds checking to borrow method with offsetThe updated borrow method doesn't validate that the offset is within the buffer's bounds. This could lead to undefined behavior if the offset exceeds m_size.
PCSX::Slice PCSX::BufferFile::borrow(size_t offset) { Slice ret; + if (offset >= m_size) { + return ret; // Return empty slice if offset is out of bounds + } ret.borrow(m_data + offset, m_size - offset); return ret; }tools/authoring/README.md (1)
1-44: Well-structured documentation for the new CD Authoring tool.The README provides clear and comprehensive documentation explaining the tool's purpose, usage, and JSON structure. It effectively communicates how to use the tool to create bootable PlayStation CD images.
Consider these minor grammatical improvements:
- Line 5: Change "requires to have" to "requires having"
- Line 37: Add "the" before "base directory"
- Line 37: Consider using "stored in" instead of "stored into"
🧰 Tools
🪛 LanguageTool
[grammar] ~5-~5: Did you mean “having”? Or maybe you should add a pronoun? In active voice, ‘require’ + ‘to’ takes an object, usually a pronoun.
Context: ...means the archive manager only requires to have the UCL-NRV2E decompressor registered. ...(ALLOW_TO)
[uncategorized] ~37-~37: You might be missing the article “the” here.
Context: ...ll the file lookups will be relative to base directory, which is the same as the jso...(AI_EN_LECTOR_MISSING_DETERMINER_THE)
[uncategorized] ~37-~37: The preposition “in” seems more likely in this position than the preposition “into”.
Context: ...n't specified. The files will be stored into the archive in their order of appearanc...(AI_EN_LECTOR_REPLACEMENT_PREPOSITION_INTO_IN)
[uncategorized] ~41-~41: Loose punctuation mark.
Context: ...ollowing command line arguments: --o: The output file name for the CD image. ...(UNLIKELY_OPENING_PUNCTUATION)
[uncategorized] ~42-~42: Loose punctuation mark.
Context: ...or the CD image. Mandatory. --license: The license file to be used. Optional. ...(UNLIKELY_OPENING_PUNCTUATION)
[uncategorized] ~43-~43: Loose punctuation mark.
Context: ...ty license on the CD image. --basedir: The base directory for the files. Optio...(UNLIKELY_OPENING_PUNCTUATION)
[uncategorized] ~44-~44: Loose punctuation mark.
Context: ...e the json file is located. --threads: The number of threads to use for compre...(UNLIKELY_OPENING_PUNCTUATION)
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (35)
.github/filter-support/Makefile-filtered(1 hunks).gitignore(1 hunks)Makefile(1 hunks)azure-pipelines-cli.yml(2 hunks)azure-pipelines.yml(2 hunks)src/cdrom/common.h(0 hunks)src/cdrom/file.cc(5 hunks)src/cdrom/file.h(3 hunks)src/cdrom/iso9660-builder.h(0 hunks)src/cdrom/iso9660-reader.cc(1 hunks)src/cdrom/iso9660-reader.h(1 hunks)src/core/luaiso.cc(3 hunks)src/core/web-server.cc(3 hunks)src/mips/common/util/util.h(1 hunks)src/support/binstruct.h(1 hunks)src/support/file.cc(2 hunks)src/support/file.h(3 hunks)src/supportpsx/binffi.lua(1 hunks)src/supportpsx/iec-60908b.h(1 hunks)src/supportpsx/iso9660-builder.cc(4 hunks)src/supportpsx/iso9660-builder.h(1 hunks)src/supportpsx/iso9660-lowlevel.h(3 hunks)tools/authoring/README.md(1 hunks)tools/authoring/authoring.cc(1 hunks)tools/exe2iso/exe2iso.cc(2 hunks)tools/modconv/modconv.cc(2 hunks)tools/ps1-packer/ps1-packer.cc(2 hunks)vsprojects/authoring/authoring.vcxproj(1 hunks)vsprojects/authoring/authoring.vcxproj.filters(1 hunks)vsprojects/cdrom/cdrom.vcxproj(0 hunks)vsprojects/cdrom/cdrom.vcxproj.filters(0 hunks)vsprojects/exe2iso/exe2iso.vcxproj(1 hunks)vsprojects/pcsx-redux.sln(3 hunks)vsprojects/supportpsx/supportpsx.vcxproj(2 hunks)vsprojects/supportpsx/supportpsx.vcxproj.filters(2 hunks)
💤 Files with no reviewable changes (4)
- src/cdrom/common.h
- src/cdrom/iso9660-builder.h
- vsprojects/cdrom/cdrom.vcxproj
- vsprojects/cdrom/cdrom.vcxproj.filters
🚧 Files skipped from review as they are similar to previous changes (23)
- azure-pipelines.yml
- src/cdrom/iso9660-reader.h
- .gitignore
- src/supportpsx/iec-60908b.h
- src/mips/common/util/util.h
- src/supportpsx/binffi.lua
- src/cdrom/iso9660-reader.cc
- vsprojects/exe2iso/exe2iso.vcxproj
- vsprojects/supportpsx/supportpsx.vcxproj.filters
- tools/ps1-packer/ps1-packer.cc
- vsprojects/authoring/authoring.vcxproj.filters
- vsprojects/authoring/authoring.vcxproj
- azure-pipelines-cli.yml
- src/core/web-server.cc
- src/cdrom/file.h
- src/core/luaiso.cc
- src/supportpsx/iso9660-lowlevel.h
- vsprojects/supportpsx/supportpsx.vcxproj
- src/supportpsx/iso9660-builder.h
- tools/modconv/modconv.cc
- Makefile
- src/cdrom/file.cc
- .github/filter-support/Makefile-filtered
🧰 Additional context used
🪛 LanguageTool
tools/authoring/README.md
[grammar] ~5-~5: Did you mean “having”? Or maybe you should add a pronoun? In active voice, ‘require’ + ‘to’ takes an object, usually a pronoun.
Context: ...means the archive manager only requires to have the UCL-NRV2E decompressor registered. ...
(ALLOW_TO)
[uncategorized] ~37-~37: You might be missing the article “the” here.
Context: ...ll the file lookups will be relative to base directory, which is the same as the jso...
(AI_EN_LECTOR_MISSING_DETERMINER_THE)
[uncategorized] ~37-~37: The preposition “in” seems more likely in this position than the preposition “into”.
Context: ...n't specified. The files will be stored into the archive in their order of appearanc...
(AI_EN_LECTOR_REPLACEMENT_PREPOSITION_INTO_IN)
[uncategorized] ~41-~41: Loose punctuation mark.
Context: ...ollowing command line arguments: - -o: The output file name for the CD image. ...
(UNLIKELY_OPENING_PUNCTUATION)
[uncategorized] ~42-~42: Loose punctuation mark.
Context: ...or the CD image. Mandatory. - -license: The license file to be used. Optional. ...
(UNLIKELY_OPENING_PUNCTUATION)
[uncategorized] ~43-~43: Loose punctuation mark.
Context: ...ty license on the CD image. - -basedir: The base directory for the files. Optio...
(UNLIKELY_OPENING_PUNCTUATION)
[uncategorized] ~44-~44: Loose punctuation mark.
Context: ...e the json file is located. - -threads: The number of threads to use for compre...
(UNLIKELY_OPENING_PUNCTUATION)
🪛 GitHub Check: CodeScene Cloud Delta Analysis (main)
tools/authoring/authoring.cc
[warning] 93-493: ❌ New issue: Complex Method
main has a cyclomatic complexity of 53, threshold = 9. 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] 102-102: ❌ New issue: Complex Conditional
main has 1 complex conditionals with 2 branches, threshold = 2. A complex conditional is an expression inside a branch (e.g. if, for, while) which consists of multiple, logical operators such as AND/OR. The more logical operators in an expression, the more severe the code smell.
[warning] 93-493: ❌ New issue: Bumpy Road Ahead
main has 5 blocks with nested conditional logic. Any nesting of 2 or deeper is considered. Threshold is one single, nested block per function. The Bumpy Road code smell is a function that contains multiple chunks of nested conditional logic. The deeper the nesting and the more bumps, the lower the code health.
[warning] 93-493: ❌ New issue: Deep, Nested Complexity
main has a nested complexity depth of 4, threshold = 4. This function contains deeply nested logic such as if statements and/or loops. The deeper the nesting, the lower the code health.
tools/exe2iso/exe2iso.cc
[warning] 98-194: ❌ Getting worse: Complex Method
main already has high cyclomatic complexity, and now it increases in Lines of Code from 131 to 132. 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] 98-194: ❌ Getting worse: Complex Method
main already has high cyclomatic complexity, and now it increases in Lines of Code from 131 to 132. 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 (1)
- GitHub Check: Codacy Static Code Analysis
🔇 Additional comments (21)
src/support/binstruct.h (2)
152-155: Improved assignment operator implementationThe change from returning
CString<S>toCString&aligns with C++ best practices for assignment operators, allowing for proper chaining of assignments likea = b = c.
156-163: Enhanced string handling with padding supportThe addition of the padding parameter and implementation changes to the
setmethod significantly improve string handling:
- Using
std::min()to determine the correct copy size- Adding padding capability for cases where the input string is shorter than the fixed size
- Ensuring proper memory initialization throughout the buffer
This change maintains backwards compatibility while adding useful functionality.
src/supportpsx/iso9660-builder.cc (4)
1-25: License updated to MITThe license has been correctly updated from GNU General Public License to MIT License, which is more permissive and aligns with the project's licensing direction.
42-42: Successfully qualified SectorMode with IEC60908b namespaceAll references to SectorMode enum have been updated to use the fully qualified name (IEC60908b::SectorMode), which improves code clarity by explicitly showing where the enum is defined.
Also applies to: 48-49, 57-57
61-62: Method signature properly updatedThe method signature for writeSectorAt has been properly updated to use the IEC60908b namespace for the SectorMode parameter type.
69-69: Consistent enum namespace usage in switch casesAll switch cases correctly use the fully qualified enum values (IEC60908b::SectorMode), maintaining consistency throughout the codebase.
Also applies to: 72-72, 81-81, 95-95
src/support/file.h (3)
344-344: Added Borrow enum for memory managementNew enum provides a clear semantic indicator for borrowed memory usage in BufferFile.
356-359: Well-documented constructor for borrowed memoryThe documentation clearly explains that the buffer will be writable but won't be enlarged if the write goes past the end, which is important for users to understand the limitations.
377-377: Enhanced borrow method with offset parameterAdded an optional offset parameter to the borrow method, which increases flexibility by allowing access to specific portions of the buffer without duplicating memory.
src/support/file.cc (2)
55-59: Implementation of the Borrow constructorThe constructor correctly implements the borrowing behavior by setting
m_ownedto false while still making the buffer writable (RW_SEEKABLE).
154-160: Updated dup method to handle ownership correctlyThe condition has been enhanced to check both ownership and writability before deciding how to duplicate the buffer.
tools/exe2iso/exe2iso.cc (2)
98-99: Improved code safety with constant variables.Using
const autoinstead ofautofor variables that shouldn't be modified after initialization is a good practice that prevents accidental modifications and improves code clarity.🧰 Tools
🪛 GitHub Check: CodeScene Cloud Delta Analysis (main)
[warning] 98-194: ❌ Getting worse: Complex Method
main already has high cyclomatic complexity, and now it increases in Lines of Code from 131 to 132. 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.
194-194: Fixed header initialization in license-free path.Important bug fix that ensures the sector header is properly initialized even when no license is written. This prevents potential issues with malformed headers in the generated ISO.
🧰 Tools
🪛 GitHub Check: CodeScene Cloud Delta Analysis (main)
[warning] 98-194: ❌ Getting worse: Complex Method
main already has high cyclomatic complexity, and now it increases in Lines of Code from 131 to 132. 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.tools/authoring/authoring.cc (4)
46-56: Well-designed templated buffer writing function.The
writeToBuffertemplate handles endianness conversions correctly and provides a clean interface for writing different integral types to buffers.
60-91: Well-structured IndexEntry union with bitfield handling.The union provides a clean interface for manipulating compressed entry data with proper bit field handling, and the static_assert confirms the expected size.
235-242: Good implementation of work unit with semaphore synchronization.The WorkUnit structure uses a binary semaphore for thread synchronization, allowing efficient parallel compression of files while maintaining control over the workflow.
198-201: Proper validation for file count limits.Good addition of a check against the maximum sector count to prevent buffer overflows, addressing a previous review comment.
vsprojects/pcsx-redux.sln (4)
112-113: Properly integrated authoring tool into Visual Studio solution.The new authoring project has been added with the correct project type and GUID format.
523-532: Complete configuration for all build types.All necessary build configurations (Debug, Release, ReleaseCLI, ReleaseWithClangCL, ReleaseWithTracy) have been properly added for the authoring project, consistent with other tools in the solution.
580-580: Correct project organization in solution hierarchy.The authoring project is appropriately nested under the tools folder, maintaining the solution's organizational structure.
584-584: Updated solution GUID.The solution GUID has been properly updated, which is necessary when modifying a Visual Studio solution file.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
tools/authoring/authoring.cc (1)
93-493: 🛠️ Refactor suggestionRefactor
main()to reduce complexity.This function has grown quite large and has a cyclomatic complexity of 53. Extracting logical sections into helper functions would improve readability, maintainability, and testability.
Consider breaking down the main function into these logical components:
- Argument parsing and validation
- JSON loading and validation
- File loading and preparation
- Compression work (thread creation, monitoring)
- ISO image generation
Example refactoring pattern:
bool parseArguments(int argc, char** argv, /* out params */); bool loadAndValidateJson(const std::string& path, /* out params */); bool loadExecutable(/* params */); bool compressFiles(/* params */); bool generateIsoImage(/* params */); int main(int argc, char** argv) { // Parse arguments if (!parseArguments(argc, argv, ...)) return -1; // Load and validate JSON if (!loadAndValidateJson(...)) return -1; // And so on... return 0; }🧰 Tools
🪛 GitHub Check: CodeScene Cloud Delta Analysis (main)
[warning] 93-493: ❌ New issue: Complex Method
main has a cyclomatic complexity of 53, threshold = 9. 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] 102-102: ❌ New issue: Complex Conditional
main has 1 complex conditionals with 2 branches, threshold = 2. A complex conditional is an expression inside a branch (e.g. if, for, while) which consists of multiple, logical operators such as AND/OR. The more logical operators in an expression, the more severe the code smell.
[warning] 93-493: ❌ New issue: Bumpy Road Ahead
main has 5 blocks with nested conditional logic. Any nesting of 2 or deeper is considered. Threshold is one single, nested block per function. The Bumpy Road code smell is a function that contains multiple chunks of nested conditional logic. The deeper the nesting and the more bumps, the lower the code health.
[warning] 93-493: ❌ New issue: Deep, Nested Complexity
main has a nested complexity depth of 4, threshold = 4. This function contains deeply nested logic such as if statements and/or loops. The deeper the nesting, the lower the code health.
🧹 Nitpick comments (7)
tools/exe2iso/exe2iso.cc (1)
85-194: Consider refactoring the main function.The static analysis indicates that the main function is complex and growing in size. Consider breaking it down into smaller functions to improve readability and maintainability.
You could extract functionality into separate methods, for example:
processLicenseFile()generateISOStructure()writeExeToISO()writeAdditionalData()🧰 Tools
🪛 GitHub Check: CodeScene Cloud Delta Analysis (main)
[warning] 98-194: ❌ Getting worse: Complex Method
main already has high cyclomatic complexity, and now it increases in Lines of Code from 131 to 132. 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.tools/authoring/authoring.cc (2)
102-103: Simplify complex conditional for better readability.The conditional combines multiple checks, making it harder to understand at a glance.
Consider breaking this into separate conditions:
-if (asksForHelp || !hasExactlyOneInput || !hasOutput) { +if (asksForHelp) { + // Show help + fmt::print(R"(...)", argv[0]); + return 0; // Return success when help is requested +} + +if (!hasExactlyOneInput) { + fmt::print("Error: Exactly one input JSON file must be specified.\n"); + fmt::print(R"(...)", argv[0]); + return -1; +} + +if (!hasOutput) { + fmt::print("Error: Output file must be specified with -o option.\n"); + fmt::print(R"(...)", argv[0]); + return -1; +}🧰 Tools
🪛 GitHub Check: CodeScene Cloud Delta Analysis (main)
[warning] 102-102: ❌ New issue: Complex Conditional
main has 1 complex conditionals with 2 branches, threshold = 2. A complex conditional is an expression inside a branch (e.g. if, for, while) which consists of multiple, logical operators such as AND/OR. The more logical operators in an expression, the more severe the code smell.
261-336: Consider managed thread lifecycle.The code uses detached threads which can be problematic if the process terminates unexpectedly. While the error handling is present (via the failed flag), consider using a thread pool or joinable threads for better lifecycle management.
You could use a std::vector of std::thread objects and join them at the end of processing:
std::vector<std::thread> threads; threads.reserve(threadCount); for (unsigned i = 0; i < threadCount; i++) { threads.emplace_back([&]() { // Thread work (existing lambda code) }); } // Join all threads when done for (auto& thread : threads) { thread.join(); }tools/authoring/README.md (4)
5-5: Correct grammar in documentation.The phrasing "requires to have" is grammatically awkward.
-The generated CD image will have its archive file stored at the sector 23, and as such, the archive manager may be initialized using this LBA. The filenames will be hashed using DJB2. Each entry may be compressed using the UCL-NRV2E algorithm. The LZ4 algorithm will not be used. This means the archive manager only requires to have the UCL-NRV2E decompressor registered. +The generated CD image will have its archive file stored at the sector 23, and as such, the archive manager may be initialized using this LBA. The filenames will be hashed using DJB2. Each entry may be compressed using the UCL-NRV2E algorithm. The LZ4 algorithm will not be used. This means the archive manager only requires having the UCL-NRV2E decompressor registered.🧰 Tools
🪛 LanguageTool
[grammar] ~5-~5: Did you mean “having”? Or maybe you should add a pronoun? In active voice, ‘require’ + ‘to’ takes an object, usually a pronoun.
Context: ...means the archive manager only requires to have the UCL-NRV2E decompressor registered. ...(ALLOW_TO)
37-37: Add missing article "the" for improved readability.The sentence is missing the definite article before "base directory".
-All the file lookups will be relative to base directory, which is the same as the json file if the option `-basedir` isn't specified. The files will be stored into the archive in their order of appearance. The `executable` field is mandatory, and it should point to a valid PSX executable file. The `files` field is an array of objects, each containing a `path` field that points to a file to be included in the CD image. The attached `name` field for each object is optional, and will default to the `path` field. It represents the final string which will be hashed into the archive index, and can be virtually any string. The `pvd` field is an optional object that contains the PVD (Primary Volume Descriptor) information for the CD image. All fields in the `pvd` object are optional. The `system_id` field defaults to "PLAYSTATION", and all other fields default to empty strings. The `volume_id` field is often used by emulators to identify the CD image, and it is recommended to set it to a unique value. +All the file lookups will be relative to the base directory, which is the same as the json file if the option `-basedir` isn't specified. The files will be stored into the archive in their order of appearance. The `executable` field is mandatory, and it should point to a valid PSX executable file. The `files` field is an array of objects, each containing a `path` field that points to a file to be included in the CD image. The attached `name` field for each object is optional, and will default to the `path` field. It represents the final string which will be hashed into the archive index, and can be virtually any string. The `pvd` field is an optional object that contains the PVD (Primary Volume Descriptor) information for the CD image. All fields in the `pvd` object are optional. The `system_id` field defaults to "PLAYSTATION", and all other fields default to empty strings. The `volume_id` field is often used by emulators to identify the CD image, and it is recommended to set it to a unique value.🧰 Tools
🪛 LanguageTool
[uncategorized] ~37-~37: You might be missing the article “the” here.
Context: ...ll the file lookups will be relative to base directory, which is the same as the jso...(AI_EN_LECTOR_MISSING_DETERMINER_THE)
[uncategorized] ~37-~37: The preposition “in” seems more likely in this position than the preposition “into”.
Context: ...n't specified. The files will be stored into the archive in their order of appearanc...(AI_EN_LECTOR_REPLACEMENT_PREPOSITION_INTO_IN)
37-37: Use preposition "in" instead of "into" for better flow.The current wording is slightly awkward.
-All the file lookups will be relative to base directory, which is the same as the json file if the option `-basedir` isn't specified. The files will be stored into the archive in their order of appearance. +All the file lookups will be relative to base directory, which is the same as the json file if the option `-basedir` isn't specified. The files will be stored in the archive in their order of appearance.🧰 Tools
🪛 LanguageTool
[uncategorized] ~37-~37: You might be missing the article “the” here.
Context: ...ll the file lookups will be relative to base directory, which is the same as the jso...(AI_EN_LECTOR_MISSING_DETERMINER_THE)
[uncategorized] ~37-~37: The preposition “in” seems more likely in this position than the preposition “into”.
Context: ...n't specified. The files will be stored into the archive in their order of appearanc...(AI_EN_LECTOR_REPLACEMENT_PREPOSITION_INTO_IN)
41-44: Improve formatting of command-line arguments list.The list formatting could be improved for better readability when rendered as markdown.
Consider adding indentation to list items:
-The tool accepts the following command line arguments: - -- `-o`: The output file name for the CD image. Mandatory. -- `-license`: The license file to be used. Optional. Point to an existing valid PlayStation 1 disc, or an official license file. The tool will use the license file to generate the CD image. If not provided, the tool will generate an empty license on the CD image. -- `-basedir`: The base directory for the files. Optional. Default is the directory where the json file is located. -- `-threads`: The number of threads to use for compression. Optional. Default is the number of available CPU cores. +The tool accepts the following command line arguments: + +- `-o`: The output file name for the CD image. Mandatory. +- `-license`: The license file to be used. Optional. Point to an existing valid PlayStation 1 disc, or an official license file. The tool will use the license file to generate the CD image. If not provided, the tool will generate an empty license on the CD image. +- `-basedir`: The base directory for the files. Optional. Default is the directory where the json file is located. +- `-threads`: The number of threads to use for compression. Optional. Default is the number of available CPU cores.🧰 Tools
🪛 LanguageTool
[uncategorized] ~41-~41: Loose punctuation mark.
Context: ...ollowing command line arguments: --o: The output file name for the CD image. ...(UNLIKELY_OPENING_PUNCTUATION)
[uncategorized] ~42-~42: Loose punctuation mark.
Context: ...or the CD image. Mandatory. --license: The license file to be used. Optional. ...(UNLIKELY_OPENING_PUNCTUATION)
[uncategorized] ~43-~43: Loose punctuation mark.
Context: ...ty license on the CD image. --basedir: The base directory for the files. Optio...(UNLIKELY_OPENING_PUNCTUATION)
[uncategorized] ~44-~44: Loose punctuation mark.
Context: ...e the json file is located. --threads: The number of threads to use for compre...(UNLIKELY_OPENING_PUNCTUATION)
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (35)
.github/filter-support/Makefile-filtered(1 hunks).gitignore(1 hunks)Makefile(1 hunks)azure-pipelines-cli.yml(2 hunks)azure-pipelines.yml(2 hunks)src/cdrom/common.h(0 hunks)src/cdrom/file.cc(5 hunks)src/cdrom/file.h(3 hunks)src/cdrom/iso9660-builder.h(0 hunks)src/cdrom/iso9660-reader.cc(1 hunks)src/cdrom/iso9660-reader.h(1 hunks)src/core/luaiso.cc(3 hunks)src/core/web-server.cc(3 hunks)src/mips/common/util/util.h(1 hunks)src/support/binstruct.h(1 hunks)src/support/file.cc(2 hunks)src/support/file.h(3 hunks)src/supportpsx/binffi.lua(1 hunks)src/supportpsx/iec-60908b.h(1 hunks)src/supportpsx/iso9660-builder.cc(4 hunks)src/supportpsx/iso9660-builder.h(1 hunks)src/supportpsx/iso9660-lowlevel.h(3 hunks)tools/authoring/README.md(1 hunks)tools/authoring/authoring.cc(1 hunks)tools/exe2iso/exe2iso.cc(2 hunks)tools/modconv/modconv.cc(2 hunks)tools/ps1-packer/ps1-packer.cc(2 hunks)vsprojects/authoring/authoring.vcxproj(1 hunks)vsprojects/authoring/authoring.vcxproj.filters(1 hunks)vsprojects/cdrom/cdrom.vcxproj(0 hunks)vsprojects/cdrom/cdrom.vcxproj.filters(0 hunks)vsprojects/exe2iso/exe2iso.vcxproj(1 hunks)vsprojects/pcsx-redux.sln(3 hunks)vsprojects/supportpsx/supportpsx.vcxproj(2 hunks)vsprojects/supportpsx/supportpsx.vcxproj.filters(2 hunks)
💤 Files with no reviewable changes (4)
- src/cdrom/common.h
- src/cdrom/iso9660-builder.h
- vsprojects/cdrom/cdrom.vcxproj.filters
- vsprojects/cdrom/cdrom.vcxproj
🚧 Files skipped from review as they are similar to previous changes (20)
- .gitignore
- src/cdrom/iso9660-reader.h
- azure-pipelines.yml
- vsprojects/authoring/authoring.vcxproj
- src/cdrom/iso9660-reader.cc
- vsprojects/authoring/authoring.vcxproj.filters
- src/supportpsx/binffi.lua
- vsprojects/exe2iso/exe2iso.vcxproj
- .github/filter-support/Makefile-filtered
- vsprojects/supportpsx/supportpsx.vcxproj.filters
- azure-pipelines-cli.yml
- src/supportpsx/iec-60908b.h
- src/cdrom/file.h
- src/support/binstruct.h
- tools/ps1-packer/ps1-packer.cc
- tools/modconv/modconv.cc
- src/core/web-server.cc
- src/supportpsx/iso9660-builder.h
- vsprojects/supportpsx/supportpsx.vcxproj
- src/core/luaiso.cc
🧰 Additional context used
🪛 LanguageTool
tools/authoring/README.md
[grammar] ~5-~5: Did you mean “having”? Or maybe you should add a pronoun? In active voice, ‘require’ + ‘to’ takes an object, usually a pronoun.
Context: ...means the archive manager only requires to have the UCL-NRV2E decompressor registered. ...
(ALLOW_TO)
[uncategorized] ~37-~37: You might be missing the article “the” here.
Context: ...ll the file lookups will be relative to base directory, which is the same as the jso...
(AI_EN_LECTOR_MISSING_DETERMINER_THE)
[uncategorized] ~37-~37: The preposition “in” seems more likely in this position than the preposition “into”.
Context: ...n't specified. The files will be stored into the archive in their order of appearanc...
(AI_EN_LECTOR_REPLACEMENT_PREPOSITION_INTO_IN)
[uncategorized] ~41-~41: Loose punctuation mark.
Context: ...ollowing command line arguments: - -o: The output file name for the CD image. ...
(UNLIKELY_OPENING_PUNCTUATION)
[uncategorized] ~42-~42: Loose punctuation mark.
Context: ...or the CD image. Mandatory. - -license: The license file to be used. Optional. ...
(UNLIKELY_OPENING_PUNCTUATION)
[uncategorized] ~43-~43: Loose punctuation mark.
Context: ...ty license on the CD image. - -basedir: The base directory for the files. Optio...
(UNLIKELY_OPENING_PUNCTUATION)
[uncategorized] ~44-~44: Loose punctuation mark.
Context: ...e the json file is located. - -threads: The number of threads to use for compre...
(UNLIKELY_OPENING_PUNCTUATION)
🪛 GitHub Check: CodeScene Cloud Delta Analysis (main)
tools/authoring/authoring.cc
[warning] 93-493: ❌ New issue: Complex Method
main has a cyclomatic complexity of 53, threshold = 9. 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] 102-102: ❌ New issue: Complex Conditional
main has 1 complex conditionals with 2 branches, threshold = 2. A complex conditional is an expression inside a branch (e.g. if, for, while) which consists of multiple, logical operators such as AND/OR. The more logical operators in an expression, the more severe the code smell.
[warning] 93-493: ❌ New issue: Bumpy Road Ahead
main has 5 blocks with nested conditional logic. Any nesting of 2 or deeper is considered. Threshold is one single, nested block per function. The Bumpy Road code smell is a function that contains multiple chunks of nested conditional logic. The deeper the nesting and the more bumps, the lower the code health.
[warning] 93-493: ❌ New issue: Deep, Nested Complexity
main has a nested complexity depth of 4, threshold = 4. This function contains deeply nested logic such as if statements and/or loops. The deeper the nesting, the lower the code health.
tools/exe2iso/exe2iso.cc
[warning] 98-194: ❌ Getting worse: Complex Method
main already has high cyclomatic complexity, and now it increases in Lines of Code from 131 to 132. 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] 98-194: ❌ Getting worse: Complex Method
main already has high cyclomatic complexity, and now it increases in Lines of Code from 131 to 132. 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 (x86_64-linux)
- GitHub Check: pcsx-redux (aarch64-linux)
- GitHub Check: Codacy Static Code Analysis
- GitHub Check: toolchain
- GitHub Check: aur-build
- GitHub Check: asan
- GitHub Check: coverage
- GitHub Check: macos-build-and-test-toolchain
- GitHub Check: cross-arm64
- GitHub Check: build
🔇 Additional comments (35)
src/mips/common/util/util.h (1)
57-64: Added MIPS-specific unaligned memory access function.The new
load32Unalignedfunction provides an efficient way to handle unaligned 32-bit memory accesses on MIPS architecture using__builtin_memcpy. This is a good addition as it will help optimize memory operations when dealing with unaligned data on MIPS platforms.src/support/file.h (3)
344-344: Added Borrow enum for buffer management.This enum addition provides a clear indicator for the borrowing memory pattern, improving API clarity.
356-359: Good documentation for the new borrowed buffer functionality.The comments clearly explain that borrowed buffers can be written to but won't expand beyond their original size, which is important behavior to document.
377-377: Enhanced borrow method with offset parameter.Adding the offset parameter allows for more flexible slice creation when borrowing buffer data, which is a useful extension of the existing functionality.
tools/exe2iso/exe2iso.cc (2)
98-99: Improved variable immutability with const.Making
outputandinputsconstants prevents accidental modification, enhancing code safety.🧰 Tools
🪛 GitHub Check: CodeScene Cloud Delta Analysis (main)
[warning] 98-194: ❌ Getting worse: Complex Method
main already has high cyclomatic complexity, and now it increases in Lines of Code from 131 to 132. 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.
194-194: Fixed potential header initialization issue.Added
makeHeaderOnce(sector)call ensures that the sector header is properly initialized even when no license file is written.🧰 Tools
🪛 GitHub Check: CodeScene Cloud Delta Analysis (main)
[warning] 98-194: ❌ Getting worse: Complex Method
main already has high cyclomatic complexity, and now it increases in Lines of Code from 131 to 132. 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.src/support/file.cc (3)
55-59: Implemented buffer borrowing constructor.The new constructor properly initializes a
BufferFilethat references existing memory without taking ownership. This is useful for cases where memory is managed elsewhere but still needs to be accessed through theFileinterface.
154-160: Updated dup() to handle owned vs. borrowed buffers.The modification correctly checks if the buffer is owned before determining how to duplicate it, preserving the ownership semantics in the duplicated file.
162-166: Enhanced borrow method with offset support.The updated implementation properly adjusts the borrowed slice to start from the specified offset, providing more flexibility when working with buffer segments.
Makefile (2)
168-169: Added new source files to support CD authoringThe
SUPPORT_SRCSvariable now includessrc/supportpsx/iec-60908b.ccandsrc/supportpsx/iso9660-builder.cc, which are essential components for the CD authoring tool. The addition ofsrc/support/container-file.ccsuggests improved file handling capabilities.
175-175: Added new authoring tool to the build systemThe
authoringtool has been added to the beginning of theTOOLSlist, making it available as part of the build process. This aligns with the PR objective of adding CD authoring functionality.src/supportpsx/iso9660-builder.cc (9)
1-25: License updated to MITThe license has been changed from GPL to MIT, which provides more permissive terms for usage and distribution.
42-43: Updated sector mode enum referenceThe code now uses the fully qualified
IEC60908b::SectorMode::M2_FORM1instead of a potentially ambiguousSectorMode::M2_FORM1. This improves code clarity and namespace organization.
48-50: Updated sector mode enum referenceConsistent use of
IEC60908b::SectorMode::RAWmaintains the namespace organization pattern.
57-58: Updated sector mode enum referenceConsistent use of
IEC60908b::SectorMode::M2_FORM1maintains the namespace organization pattern.
61-62: Updated function signature to use namespaced enumThe parameter type has been changed from
SectorMode modetoIEC60908b::SectorMode modeto be consistent with the enum namespace refactoring.
69-70: Updated sector mode enum in switch statementThe switch case now uses
IEC60908b::SectorMode::RAWto maintain consistent namespace usage.
72-73: Updated sector mode enum in switch statementThe switch case now uses
IEC60908b::SectorMode::M2_RAWto maintain consistent namespace usage.
81-82: Updated sector mode enum in switch statementThe switch case now uses
IEC60908b::SectorMode::M2_FORM1to maintain consistent namespace usage.
95-96: Updated sector mode enum in switch statementThe switch case now uses
IEC60908b::SectorMode::M2_FORM2to maintain consistent namespace usage.src/supportpsx/iso9660-lowlevel.h (3)
1-25: License updated to MITThe license has been changed from GPL to MIT, which provides more permissive terms for usage and distribution.
34-34: Modernized namespace declarationThe namespace declaration has been updated from the nested style
namespace PCSX { namespace ISO9660LowLevel {to the more concisenamespace PCSX::ISO9660LowLevel {, which is a more modern C++ approach.
136-136: Updated namespace closingThe namespace closing has been simplified to match the modernized opening declaration.
src/cdrom/file.cc (6)
27-27: Updated constructor parameter to use namespaced enumThe constructor now uses
IEC60908b::SectorMode modeinstead ofSectorMode modeto be consistent with the enum namespace refactoring.
34-36: Updated enum referencesReferences to
SectorMode::GUESSandSectorMode::RAWhave been updated toIEC60908b::SectorMode::GUESSandIEC60908b::SectorMode::RAWrespectively.
51-52: Updated enum referenceReference to
SectorMode::M1has been updated toIEC60908b::SectorMode::M1.
60-63: Updated enum referencesReferences to
SectorMode::M2_FORM2andSectorMode::M2_FORM1have been updated toIEC60908b::SectorMode::M2_FORM2andIEC60908b::SectorMode::M2_FORM1respectively.
77-77: Updated enum references in conditionThe condition now checks
IEC60908b::SectorMode::M2_FORM1andIEC60908b::SectorMode::M2_FORM2instead of the previously unnamespaced versions.
203-205: Updated enum references in switch casesThe switch cases now use
IEC60908b::SectorMode::M2_FORM1andIEC60908b::SectorMode::M2_FORM2to maintain consistent namespace usage.tools/authoring/authoring.cc (2)
46-56: Well-designed buffer writing utility.This template function properly handles writing integral values to potentially unaligned memory buffers with endianness control, which is essential for CD format compatibility.
60-89: Efficient bit-packed structure for index entries.The IndexEntry union effectively utilizes bit packing to store file metadata in a compact 16-byte structure, with proper accessor methods and size verification.
vsprojects/pcsx-redux.sln (4)
112-113: Project integration looks good.The authoring project has been properly added to the solution with a unique GUID.
523-532: Build configurations correctly set up.All necessary build configurations for the authoring project are properly defined and consistent with other projects in the solution.
580-580: Project correctly nested in tools folder.The authoring project is properly added to the tools folder structure.
584-584: Solution GUID update is expected.The SolutionGuid change is a normal part of adding a new project to the solution.
nicolasnoble
force-pushed
the
authoring
branch
2 times, most recently
from
9e3f592 to
efa694c
Compare
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
Actionable comments posted: 1
♻️ Duplicate comments (2)
tools/authoring/authoring.cc (2)
93-493: 🛠️ Refactor suggestionRefactor main() function to reduce complexity.
The main function is very large (400+ lines) with high cyclomatic complexity (53), making it difficult to maintain and test. It handles multiple responsibilities including argument parsing, file loading, multi-threading, compression, and ISO9660 image creation.
Consider extracting logical sections into separate functions:
- Argument parsing and validation
- JSON parsing and validation
- File loading and executable processing
- File compression worker function
- ISO image construction
- PVD creation
This would improve readability, maintainability, and make the code easier to test.
🧰 Tools
🪛 GitHub Check: CodeScene Cloud Delta Analysis (main)
[warning] 93-493: ❌ New issue: Complex Method
main has a cyclomatic complexity of 53, threshold = 9. 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] 102-102: ❌ New issue: Complex Conditional
main has 1 complex conditionals with 2 branches, threshold = 2. A complex conditional is an expression inside a branch (e.g. if, for, while) which consists of multiple, logical operators such as AND/OR. The more logical operators in an expression, the more severe the code smell.
[warning] 93-493: ❌ New issue: Bumpy Road Ahead
main has 5 blocks with nested conditional logic. Any nesting of 2 or deeper is considered. Threshold is one single, nested block per function. The Bumpy Road code smell is a function that contains multiple chunks of nested conditional logic. The deeper the nesting and the more bumps, the lower the code health.
[warning] 93-493: ❌ New issue: Deep, Nested Complexity
main has a nested complexity depth of 4, threshold = 4. This function contains deeply nested logic such as if statements and/or loops. The deeper the nesting, the lower the code health.
187-188:⚠️ Potential issueFix incorrect filename in error message.
The error message references the JSON input file instead of the executable file that failed to load.
-fmt::print("Unable to load file: {}\n", input); +fmt::print("Unable to load file: {}\n", executablePath);
🧹 Nitpick comments (6)
src/support/binstruct.h (1)
152-163: Consider aligning operator= with set() behaviorThe
operator=andset()methods now have different behaviors. Whileset()properly handles padding when the input string is shorter than the fixed size,operator=simply performs a direct memory copy without padding or size validation.CString &operator=(const type &v) { - memcpy(value, v.data(), S); + set(v); return *this; }tools/authoring/README.md (2)
5-5: Improve grammar in the archive manager requirements statement.The phrasing "requires to have" is grammatically awkward.
-The generated CD image will have its archive file stored at the sector 23, and as such, the archive manager may be initialized using this LBA. The filenames will be hashed using DJB2. Each entry may be compressed using the UCL-NRV2E algorithm. The LZ4 algorithm will not be used. This means the archive manager only requires to have the UCL-NRV2E decompressor registered. +The generated CD image will have its archive file stored at the sector 23, and as such, the archive manager may be initialized using this LBA. The filenames will be hashed using DJB2. Each entry may be compressed using the UCL-NRV2E algorithm. The LZ4 algorithm will not be used. This means the archive manager only requires having the UCL-NRV2E decompressor registered.🧰 Tools
🪛 LanguageTool
[grammar] ~5-~5: Did you mean “having”? Or maybe you should add a pronoun? In active voice, ‘require’ + ‘to’ takes an object, usually a pronoun.
Context: ...means the archive manager only requires to have the UCL-NRV2E decompressor registered. ...(ALLOW_TO)
37-37: Minor grammatical issues in file lookup explanation.There are two minor grammatical issues in this paragraph.
-All the file lookups will be relative to base directory, which is the same as the json file if the option `-basedir` isn't specified. The files will be stored into the archive in their order of appearance. +All the file lookups will be relative to the base directory, which is the same as the json file if the option `-basedir` isn't specified. The files will be stored in the archive in their order of appearance.🧰 Tools
🪛 LanguageTool
[uncategorized] ~37-~37: You might be missing the article “the” here.
Context: ...ll the file lookups will be relative to base directory, which is the same as the jso...(AI_EN_LECTOR_MISSING_DETERMINER_THE)
[uncategorized] ~37-~37: The preposition “in” seems more likely in this position than the preposition “into”.
Context: ...n't specified. The files will be stored into the archive in their order of appearanc...(AI_EN_LECTOR_REPLACEMENT_PREPOSITION_INTO_IN)
tools/authoring/authoring.cc (3)
255-259: Consider moving helper lambdas to module-level functions.The lambdas
createSectorHeaderandputSectorLBAcould be converted to regular functions at module level, since they don't capture any variables from their enclosing scope. This would reduce the complexity of the main function and potentially improve code reuse.+static void createSectorHeader(uint8_t sector[2352]) { + memset(sector + 1, 0xff, 10); + sector[15] = 2; + sector[18] = sector[22] = 8; +} + +static void putSectorLBA(uint8_t sector[2352], uint32_t lba) { + PCSX::IEC60908b::MSF time(lba + 150); + time.toBCD(sector + 12); +}Also applies to: 338-341
261-336: Thread management could be improved for clean termination.The current implementation creates and detaches threads, which can lead to resource leaks if the application terminates unexpectedly. Consider using a thread pool or at minimum, joining threads before program exit.
Consider using a vector to store thread objects and joining them later:
std::vector<std::thread> threads; for (unsigned i = 0; i < threadCount; i++) { threads.emplace_back([&]() { // Worker thread code... }); } // Later in the code: for (auto& t : threads) { t.join(); }
262-336: Add error handling for thread creation.The code creates threads without checking if thread creation succeeded. Although thread creation failures are rare on modern systems, it's good practice to handle them.
try { for (unsigned i = 0; i < threadCount; i++) { threads.emplace_back([&]() { // Worker thread code... }); } } catch (const std::system_error& e) { fmt::print("Failed to create threads: {}\n", e.what()); return -1; }
🛑 Comments failed to post (1)
src/supportpsx/iso9660-builder.cc (1)
1-25: 💡 Verification agent
🧩 Analysis chain
License changed from GPL to MIT.
The file's license has been updated from GNU General Public License to MIT License. This is a significant change that makes the code more permissively licensed.
Please verify that this license change is intentional and properly approved by all contributors:
🏁 Script executed:
#!/bin/bash # Check for other GPL licensed files in the project echo "Looking for files that might still be GPL licensed..." grep -r "GNU General Public License" --include="*.h" --include="*.cc" --include="*.cpp" .Length of output: 65745
License Inconsistency Detected: Confirm Licensing Transition Intent
The file
src/supportpsx/iso9660-builder.ccnow carries the MIT license header, whereas our recent search shows that many other files in the repository (e.g. insrc/cdrom/,src/core/, etc.) still include GPL license references. Please verify whether the change to MIT for this file is intentional on its own or if a repository-wide relicensing from GPL to MIT is planned. In the latter case, ensure that all affected files are updated consistently and that the change has the proper contributor approvals.
- Confirm whether the isolated MIT licensing change is deliberate or part of a broader relicense effort.
- If the project is being relicensed to MIT, update all files currently under reference to the GPL accordingly.
- Verify that all contributors have approved this licensing change.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (10)
tools/exe2iso/exe2iso.cc (1)
98-194: Consider refactoring complex main function.Static analysis indicates this function has high cyclomatic complexity, making it difficult to understand and maintain. Consider breaking it down into smaller, more focused functions.
- int main(int argc, char** argv) { + int parseCommandLine(int argc, char** argv, std::string& output, std::vector<std::string>& inputs, + std::string& license, std::string& data, uint32_t& offset, bool& pad, bool& regen) { CommandLine::args args(argc, argv); - - fmt::print(R"( -exe2iso by Nicolas "Pixel" Noble -https://github.com/grumpycoders/pcsx-redux/tree/main/tools/exe2iso/ -)"); - const auto output = args.get<std::string>("o"); const auto inputs = args.positional(); const bool asksForHelp = args.get<bool>("h").value_or(false); // ... rest of commandline parsing ... if (asksForHelp || !oneInput || !hasOutput) { // ... help text output ... return -1; } + return 0; + } + + int processFiles(const std::string& input, const std::string& output, + const std::string& license, const std::string& data, + uint32_t offset, bool pad, bool regen) { // ... file processing logic ... + } + + int main(int argc, char** argv) { + fmt::print(R"( +exe2iso by Nicolas "Pixel" Noble +https://github.com/grumpycoders/pcsx-redux/tree/main/tools/exe2iso/ +)"); + + std::string output, license, data; + std::vector<std::string> inputs; + uint32_t offset = 0; + bool pad = false, regen = false; + + int result = parseCommandLine(argc, argv, output, inputs, license, data, offset, pad, regen); + if (result != 0) return result; + + return processFiles(inputs[0], output, license, data, offset, pad, regen);🧰 Tools
🪛 GitHub Check: CodeScene Cloud Delta Analysis (main)
[warning] 98-194: ❌ Getting worse: Complex Method
main already has high cyclomatic complexity, and now it increases in Lines of Code from 131 to 132. 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.tools/authoring/authoring.cc (4)
235-242: WorkUnit structure is well-designed but could use documentation.The WorkUnit structure is well-designed for thread synchronization using the C++20 semaphore, but would benefit from documentation explaining its purpose and usage pattern.
struct WorkUnit { + // Represents a unit of work for file compression, using a semaphore for thread synchronization + // The semaphore is initially 0 and is released when the work is complete + // The fileInfo holds the JSON metadata about the file being processed WorkUnit() : semaphore(0), failed(false) {} std::binary_semaphore semaphore; std::vector<uint8_t> sectorData; nlohmann::json fileInfo; bool failed; };
261-336: Thread management could be improved.The thread creation code uses
detach()which can lead to potential issues if the program exits before threads complete. Consider usingstd::jthread(C++20) or properly joining threads in a cleanup phase.-std::thread t([&]() { +std::jthread t([&]() { // Thread function body... }); -t.detach();Additionally, add error handling for thread creation failures.
287-293: Improve error handling for compression failures.The error handling for UCL compression failures only marks the work as failed but doesn't capture the specific error code or message, making debugging difficult.
r = ucl_nrv2e_99_compress(dataIn.data(), size, dataOut.data() + 2048, &outSize, nullptr, 10, nullptr, nullptr); if (r != UCL_E_OK) { + fmt::print("UCL compression failed for file {} with error code {}\n", + workUnit.fileInfo["path"].get<std::string>(), r); workUnit.failed = true; workUnit.semaphore.release(); continue; }
394-445: PVD initialization could be refactored into a separate function.The Primary Volume Descriptor (PVD) initialization code is extensive and would benefit from being moved to a dedicated function, improving readability of the main function.
+void initializePVD(PCSX::ISO9660LowLevel::PVD& pvd, const nlohmann::json& pvdData, unsigned totalSectorCount) { + pvd.reset(); + pvd.get<PCSX::ISO9660LowLevel::PVD_TypeCode>().value = 1; + pvd.get<PCSX::ISO9660LowLevel::PVD_StdIdent>().set("CD001"); + // ...rest of initialization... +} // In main function: // PCSX::ISO9660LowLevel::PVD pvd; // initializePVD(pvd, pvdData, totalSectorCount);tools/authoring/README.md (4)
3-5: Documentation is clear but could be more detailed about the tool's purpose.The introduction provides a good overview but could clarify that this is specifically for PlayStation 1 development and provide context for when someone would need to use this tool.
# CD Authoring -This tool can be used to create a bootable CD image for the PlayStation, using a single input binary file, and a set of files to be archived into the CD image at a fixed location. See the [archive-manager.h](src/mips/psyqo-paths/archive-manager.h) file for more information about the archive format and API. +This tool creates bootable CD images for the PlayStation 1, packaging a main executable with additional files in an archive format. It's useful for PlayStation 1 development when you need to bundle multiple assets with your application. The CD image follows PlayStation standards and can be used with emulators or burned to a physical disc. See the [archive-manager.h](src/mips/psyqo-paths/archive-manager.h) file for more information about the archive format and API.🧰 Tools
🪛 LanguageTool
[grammar] ~5-~5: Did you mean “having”? Or maybe you should add a pronoun? In active voice, ‘require’ + ‘to’ takes an object, usually a pronoun.
Context: ...means the archive manager only requires to have the UCL-NRV2E decompressor registered. ...(ALLOW_TO)
5-5: Grammar improvement needed in the archive description.The sentence about the archive manager requirements has a grammatical issue.
-The generated CD image will have its archive file stored at the sector 23, and as such, the archive manager may be initialized using this LBA. The filenames will be hashed using DJB2. Each entry may be compressed using the UCL-NRV2E algorithm. The LZ4 algorithm will not be used. This means the archive manager only requires to have the UCL-NRV2E decompressor registered. +The generated CD image will have its archive file stored at sector 23, and as such, the archive manager may be initialized using this LBA. The filenames will be hashed using DJB2. Each entry may be compressed using the UCL-NRV2E algorithm. The LZ4 algorithm will not be used. This means the archive manager only needs to have the UCL-NRV2E decompressor registered.🧰 Tools
🪛 LanguageTool
[grammar] ~5-~5: Did you mean “having”? Or maybe you should add a pronoun? In active voice, ‘require’ + ‘to’ takes an object, usually a pronoun.
Context: ...means the archive manager only requires to have the UCL-NRV2E decompressor registered. ...(ALLOW_TO)
37-37: Improve clarity of file lookup description.The description of file lookups could be clearer regarding the base directory.
-All the file lookups will be relative to base directory, which is the same as the json file if the option `-basedir` isn't specified. The files will be stored into the archive in their order of appearance. The `executable` field is mandatory, and it should point to a valid PSX executable file. +All file lookups will be relative to the base directory, which defaults to the JSON file's location if the `-basedir` option isn't specified. The files will be stored in the archive in their order of appearance. The `executable` field is mandatory and should point to a valid PSX executable file.🧰 Tools
🪛 LanguageTool
[uncategorized] ~37-~37: You might be missing the article “the” here.
Context: ...ll the file lookups will be relative to base directory, which is the same as the jso...(AI_EN_LECTOR_MISSING_DETERMINER_THE)
[uncategorized] ~37-~37: The preposition “in” seems more likely in this position than the preposition “into”.
Context: ...n't specified. The files will be stored into the archive in their order of appearanc...(AI_EN_LECTOR_REPLACEMENT_PREPOSITION_INTO_IN)
41-44: Format command line arguments as a proper list.The command line arguments should be formatted consistently as a list.
-The tool accepts the following command line arguments: - -- `-o`: The output file name for the CD image. Mandatory. -- `-license`: The license file to be used. Optional. Point to an existing valid PlayStation 1 disc, or an official license file. The tool will use the license file to generate the CD image. If not provided, the tool will generate an empty license on the CD image. -- `-basedir`: The base directory for the files. Optional. Default is the directory where the json file is located. -- `-threads`: The number of threads to use for compression. Optional. Default is the number of available CPU cores. +The tool accepts the following command line arguments: + +* **`-o`**: The output file name for the CD image. **Mandatory**. +* **`-license`**: The license file to be used. *Optional*. Point to an existing valid PlayStation 1 disc, or an official license file. If not provided, the tool will generate an empty license on the CD image. +* **`-basedir`**: The base directory for the files. *Optional*. Default is the directory where the JSON file is located. +* **`-threads`**: The number of threads to use for compression. *Optional*. Default is the number of available CPU cores.🧰 Tools
🪛 LanguageTool
[uncategorized] ~41-~41: Loose punctuation mark.
Context: ...ollowing command line arguments: --o: The output file name for the CD image. ...(UNLIKELY_OPENING_PUNCTUATION)
[uncategorized] ~42-~42: Loose punctuation mark.
Context: ...or the CD image. Mandatory. --license: The license file to be used. Optional. ...(UNLIKELY_OPENING_PUNCTUATION)
[uncategorized] ~43-~43: Loose punctuation mark.
Context: ...ty license on the CD image. --basedir: The base directory for the files. Optio...(UNLIKELY_OPENING_PUNCTUATION)
[uncategorized] ~44-~44: Loose punctuation mark.
Context: ...e the json file is located. --threads: The number of threads to use for compre...(UNLIKELY_OPENING_PUNCTUATION)
src/supportpsx/iso9660-builder.h (1)
39-39: Add documentation for the writeLicense method.The
writeLicensemethod lacks implementation in the header but could benefit from clearer documentation about its purpose and expected behavior.+ /** + * @brief Writes the license information to the output file. + * + * @param licenseFile Optional IO object for the license file. If not provided, no license is written. + */ void writeLicense(IO<File> licenseFile = nullptr);
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (35)
.github/filter-support/Makefile-filtered(1 hunks).gitignore(1 hunks)Makefile(1 hunks)azure-pipelines-cli.yml(2 hunks)azure-pipelines.yml(2 hunks)src/cdrom/common.h(0 hunks)src/cdrom/file.cc(5 hunks)src/cdrom/file.h(3 hunks)src/cdrom/iso9660-builder.h(0 hunks)src/cdrom/iso9660-reader.cc(1 hunks)src/cdrom/iso9660-reader.h(1 hunks)src/core/luaiso.cc(3 hunks)src/core/web-server.cc(3 hunks)src/mips/common/util/util.h(1 hunks)src/support/binstruct.h(1 hunks)src/support/file.cc(2 hunks)src/support/file.h(3 hunks)src/supportpsx/binffi.lua(1 hunks)src/supportpsx/iec-60908b.h(1 hunks)src/supportpsx/iso9660-builder.cc(4 hunks)src/supportpsx/iso9660-builder.h(1 hunks)src/supportpsx/iso9660-lowlevel.h(3 hunks)tools/authoring/README.md(1 hunks)tools/authoring/authoring.cc(1 hunks)tools/exe2iso/exe2iso.cc(2 hunks)tools/modconv/modconv.cc(2 hunks)tools/ps1-packer/ps1-packer.cc(2 hunks)vsprojects/authoring/authoring.vcxproj(1 hunks)vsprojects/authoring/authoring.vcxproj.filters(1 hunks)vsprojects/cdrom/cdrom.vcxproj(0 hunks)vsprojects/cdrom/cdrom.vcxproj.filters(0 hunks)vsprojects/exe2iso/exe2iso.vcxproj(1 hunks)vsprojects/pcsx-redux.sln(3 hunks)vsprojects/supportpsx/supportpsx.vcxproj(2 hunks)vsprojects/supportpsx/supportpsx.vcxproj.filters(2 hunks)
💤 Files with no reviewable changes (4)
- src/cdrom/iso9660-builder.h
- src/cdrom/common.h
- vsprojects/cdrom/cdrom.vcxproj
- vsprojects/cdrom/cdrom.vcxproj.filters
🚧 Files skipped from review as they are similar to previous changes (22)
- .gitignore
- azure-pipelines.yml
- src/supportpsx/iec-60908b.h
- src/cdrom/iso9660-reader.h
- src/cdrom/iso9660-reader.cc
- src/supportpsx/binffi.lua
- azure-pipelines-cli.yml
- vsprojects/exe2iso/exe2iso.vcxproj
- .github/filter-support/Makefile-filtered
- vsprojects/authoring/authoring.vcxproj.filters
- Makefile
- src/mips/common/util/util.h
- src/cdrom/file.cc
- src/core/luaiso.cc
- src/cdrom/file.h
- vsprojects/authoring/authoring.vcxproj
- tools/ps1-packer/ps1-packer.cc
- tools/modconv/modconv.cc
- vsprojects/supportpsx/supportpsx.vcxproj
- src/support/binstruct.h
- src/supportpsx/iso9660-lowlevel.h
- vsprojects/supportpsx/supportpsx.vcxproj.filters
🧰 Additional context used
🪛 GitHub Check: CodeScene Cloud Delta Analysis (main)
tools/authoring/authoring.cc
[warning] 93-493: ❌ New issue: Complex Method
main has a cyclomatic complexity of 53, threshold = 9. 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] 102-102: ❌ New issue: Complex Conditional
main has 1 complex conditionals with 2 branches, threshold = 2. A complex conditional is an expression inside a branch (e.g. if, for, while) which consists of multiple, logical operators such as AND/OR. The more logical operators in an expression, the more severe the code smell.
[warning] 93-493: ❌ New issue: Bumpy Road Ahead
main has 5 blocks with nested conditional logic. Any nesting of 2 or deeper is considered. Threshold is one single, nested block per function. The Bumpy Road code smell is a function that contains multiple chunks of nested conditional logic. The deeper the nesting and the more bumps, the lower the code health.
[warning] 93-493: ❌ New issue: Deep, Nested Complexity
main has a nested complexity depth of 4, threshold = 4. This function contains deeply nested logic such as if statements and/or loops. The deeper the nesting, the lower the code health.
tools/exe2iso/exe2iso.cc
[warning] 98-194: ❌ Getting worse: Complex Method
main already has high cyclomatic complexity, and now it increases in Lines of Code from 131 to 132. 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] 98-194: ❌ Getting worse: Complex Method
main already has high cyclomatic complexity, and now it increases in Lines of Code from 131 to 132. 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.
🪛 LanguageTool
tools/authoring/README.md
[grammar] ~5-~5: Did you mean “having”? Or maybe you should add a pronoun? In active voice, ‘require’ + ‘to’ takes an object, usually a pronoun.
Context: ...means the archive manager only requires to have the UCL-NRV2E decompressor registered. ...
(ALLOW_TO)
[uncategorized] ~37-~37: You might be missing the article “the” here.
Context: ...ll the file lookups will be relative to base directory, which is the same as the jso...
(AI_EN_LECTOR_MISSING_DETERMINER_THE)
[uncategorized] ~37-~37: The preposition “in” seems more likely in this position than the preposition “into”.
Context: ...n't specified. The files will be stored into the archive in their order of appearanc...
(AI_EN_LECTOR_REPLACEMENT_PREPOSITION_INTO_IN)
[uncategorized] ~41-~41: Loose punctuation mark.
Context: ...ollowing command line arguments: - -o: The output file name for the CD image. ...
(UNLIKELY_OPENING_PUNCTUATION)
[uncategorized] ~42-~42: Loose punctuation mark.
Context: ...or the CD image. Mandatory. - -license: The license file to be used. Optional. ...
(UNLIKELY_OPENING_PUNCTUATION)
[uncategorized] ~43-~43: Loose punctuation mark.
Context: ...ty license on the CD image. - -basedir: The base directory for the files. Optio...
(UNLIKELY_OPENING_PUNCTUATION)
[uncategorized] ~44-~44: Loose punctuation mark.
Context: ...e the json file is located. - -threads: The number of threads to use for compre...
(UNLIKELY_OPENING_PUNCTUATION)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Codacy Static Code Analysis
- GitHub Check: aur-build
🔇 Additional comments (22)
src/support/file.h (3)
344-344: New enum Borrow added to BufferFile class.Adding a
Borrowenum provides a clear semantic marker to indicate when aBufferFileis borrowing memory rather than owning it. This approach makes the API more expressive and helps prevent memory management mistakes.
356-359: Well-documented new constructor for borrowing memory.The documentation clearly explains the behavior of this constructor - that it will allow writing to the buffer but won't expand it if writes exceed the current size. This is important for preventing buffer overflows and memory corruption.
377-377: Enhanced borrow method with offset parameter.Adding a default offset parameter to the
borrow()method provides more flexibility when accessing portions of the buffer while maintaining backward compatibility with existing code through the default value of 0.tools/exe2iso/exe2iso.cc (1)
98-99: Variables declared as const for improved immutability.Making
outputandinputsconst prevents accidental modifications, which is a good practice for variables that should not change after initialization.🧰 Tools
🪛 GitHub Check: CodeScene Cloud Delta Analysis (main)
[warning] 98-194: ❌ Getting worse: Complex Method
main already has high cyclomatic complexity, and now it increases in Lines of Code from 131 to 132. 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.src/support/file.cc (3)
55-59: Implemented new BufferFile constructor for borrowed memory.The constructor correctly initializes the necessary fields without taking ownership of the memory, which aligns with the documented behavior in the header file.
155-155: Improved dup() method to check for ownership.The updated condition
m_owned && !writable()ensures that the ownership status is considered when duplicating a file. This prevents potential issues with non-owned files being treated as owned.
162-165: Enhanced borrow method to support offset parameter.The implementation properly handles the offset parameter by adjusting both the data pointer and the size calculation, ensuring that slices created with an offset remain within valid memory boundaries.
src/supportpsx/iso9660-builder.cc (2)
1-25: Updated license from GPL to MIT.The license has been changed from GNU GPL to MIT License. This is a significant change that affects how the code can be used, redistributed, and modified.
Make sure this license change is approved by all contributors and aligns with the project's overall licensing strategy. Check that there are no dependencies or inherited code that would prevent this change.
42-43:✅ Verification successful
Updated SectorMode references to use IEC60908b namespace.
All references to
SectorModehave been updated to use the fully qualified nameIEC60908b::SectorMode. This change improves code organization by properly namespacing the enum and clarifies that it's related to the IEC60908b standard.Also applies to: 48-49, 57-57, 62-62, 69-69, 72-72, 81-81, 95-95
🏁 Script executed:
#!/bin/bash # Search for any remaining unqualified SectorMode references rg -A 2 -B 2 '\bSectorMode\b' --include="*.h" --include="*.cc" --include="*.cpp" | grep -v "IEC60908b::SectorMode"Length of output: 556
I'll re-run the verification using a revised command that uses the
-gflag instead of--include. Please run the following new script and share its output if any issues are found:Additionally, please add the `` tag to manually verify that all references to
SectorModeare now fully qualified in the codebase.
🏁 Script executed:
#!/bin/bash # Revised search for any unqualified 'SectorMode' references (excluding those using IEC60908b::SectorMode) rg -g '*.h' -g '*.cc' -g '*.cpp' '\bSectorMode\b' | grep -v "IEC60908b::SectorMode"Length of output: 257
Verified: All SectorMode references are now fully qualified
- The updated usage in files such as src/supportpsx/iso9660-builder.cc (lines 42–43, 48–49, etc.) correctly uses
IEC60908b::SectorMode.- The only remaining unqualified occurrence is in src/supportpsx/iec-60908b.h, where the
enum class SectorMode { … }is declared. This is expected since the declaration should reside within the IEC60908b namespace context.src/core/web-server.cc (3)
46-46: Approve: Updated include path for refactored ISO9660 builder.The include path has been updated from
cdrom/iso9660-builder.htosupportpsx/iso9660-builder.h, which aligns with the architectural changes being made to the codebase.
466-466: Approve: Updated SectorMode namespace.The SectorMode enum is now properly referenced with its full namespace path
PCSX::IEC60908b::SectorMode, reflecting the consolidation of sector-related definitions under the IEC60908b standard namespace.
476-476: Approve: Updated enum_cast to use the correct namespace.Consistent with the earlier change, the enum_cast now correctly references
PCSX::IEC60908b::SectorMode.tools/authoring/authoring.cc (3)
46-56: The byte-swapping utility function is well-designed.The
writeToBufferfunction effectively handles endianness conversion and buffer writing with compile-time evaluation of endianness, which is a good practice for cross-platform compatibility.
60-91: Approve: Well-structured IndexEntry union with clear bit field usage.The IndexEntry union effectively uses bit fields to compact storage requirements while providing a clean API for accessing the data. The static assertion ensures the expected size is maintained.
297-305: Approve: Good use of DJB hash for file identification.The implementation correctly uses the DJB hash for file identification, with a fallback to using the path if no explicit name is provided.
src/supportpsx/iso9660-builder.h (3)
34-47: Good class design with clean interface.The ISO9660Builder class provides a well-designed interface for creating ISO9660 file systems. Methods are concise and follow a consistent pattern.
40-43: Approve: Efficient wrapper method.The
writeSectormethod provides a convenient wrapper aroundwriteSectorAtthat automatically increments the current location. This is a good design pattern that simplifies the API.
51-51:✅ Verification successful
Initial location starts at {0, 2, 0} - verify if this is correct.
The MSF location is initialized to {0, 2, 0}, which is an unusual starting point. Verify if this is the correct starting location for a CD image.
🏁 Script executed:
#!/bin/bash # Look for other usages of MSF initialization to confirm the pattern echo "Checking typical MSF initialization values in the codebase:" grep -r "MSF.*=" --include="*.cc" --include="*.h" .Length of output: 687
Verified: Initialization of
{0, 2, 0}is CorrectAfter reviewing other instances and reflecting on typical CD image conventions (which commonly include a 2-second lead-in period), the assignment of
IEC60908b::MSF m_locationto{0, 2, 0}insrc/supportpsx/iso9660-builder.his appropriate. No changes are required.
- The codebase shows no alternative hardcoded initializations.
- The 2-second offset aligns with standard practices for CD images.
vsprojects/pcsx-redux.sln (4)
112-113: Project structure properly added for the new CD authoring tool.The new "authoring" project has been appropriately added to the solution with a unique GUID, aligning with the PR objective of adding a CD authoring tool.
523-532: Build configurations correctly defined for the authoring tool.All required build configurations (Debug, Release, ReleaseCLI, ReleaseWithClangCL) have been properly set up. The ReleaseWithTracy configuration is set to use the Release build settings, which is consistent with how other tools in the solution are configured.
580-580: Appropriate project grouping.The authoring tool is correctly nested under the "tools" group, which is consistent with its purpose as a utility tool rather than a core component.
584-584: Solution GUID updated.The SolutionGuid has been updated, which is a normal and expected change when adding a new project to the solution.
tools/authoring/authoring.cc
Outdated
| PCSX::IO<PCSX::File> pvdSector(new PCSX::BufferFile(PCSX::FileOps::READWRITE)); | ||
| PCSX::ISO9660LowLevel::PVD pvd; | ||
| pvd.reset(); | ||
| pvd.get<PCSX::ISO9660LowLevel::PVD_TypeCode>().value = 1; | ||
| pvd.get<PCSX::ISO9660LowLevel::PVD_StdIdent>().set("CD001"); | ||
| pvd.get<PCSX::ISO9660LowLevel::PVD_Version>().value = 1; | ||
| auto systemIdent = pvdData["system_id"].is_string() ? pvdData["system_id"].get<std::string>() : "PLAYSTATION"; | ||
| pvd.get<PCSX::ISO9660LowLevel::PVD_SystemIdent>().set(systemIdent, ' '); | ||
| auto volumeIdent = pvdData["volume_id"].is_string() ? pvdData["volume_id"].get<std::string>() : ""; | ||
| pvd.get<PCSX::ISO9660LowLevel::PVD_VolumeIdent>().set(volumeIdent, ' '); | ||
| pvd.get<PCSX::ISO9660LowLevel::PVD_VolumeSpaceSize>().value = totalSectorCount; | ||
| pvd.get<PCSX::ISO9660LowLevel::PVD_VolumeSpaceSizeBE>().value = totalSectorCount; | ||
| pvd.get<PCSX::ISO9660LowLevel::PVD_VolumeSetSize>().value = 1; | ||
| pvd.get<PCSX::ISO9660LowLevel::PVD_VolumeSetSizeBE>().value = 1; | ||
| pvd.get<PCSX::ISO9660LowLevel::PVD_VolumeSequenceNumber>().value = 1; | ||
| pvd.get<PCSX::ISO9660LowLevel::PVD_VolumeSequenceNumberBE>().value = 1; | ||
| pvd.get<PCSX::ISO9660LowLevel::PVD_LogicalBlockSize>().value = 2048; | ||
| pvd.get<PCSX::ISO9660LowLevel::PVD_LogicalBlockSizeBE>().value = 2048; | ||
| pvd.get<PCSX::ISO9660LowLevel::PVD_PathTableSize>().value = 10; | ||
| pvd.get<PCSX::ISO9660LowLevel::PVD_PathTableSizeBE>().value = 10; | ||
| pvd.get<PCSX::ISO9660LowLevel::PVD_LPathTableLocation>().value = 18; | ||
| pvd.get<PCSX::ISO9660LowLevel::PVD_LPathTableOptLocation>().value = 19; | ||
| pvd.get<PCSX::ISO9660LowLevel::PVD_MPathTableLocation>().value = 20; | ||
| pvd.get<PCSX::ISO9660LowLevel::PVD_MPathTableOptLocation>().value = 21; | ||
| auto& root = pvd.get<PCSX::ISO9660LowLevel::PVD_RootDir>(); | ||
| root.get<PCSX::ISO9660LowLevel::DirEntry_Length>().value = 34; | ||
| root.get<PCSX::ISO9660LowLevel::DirEntry_ExtLength>().value = 0; | ||
| root.get<PCSX::ISO9660LowLevel::DirEntry_LBA>().value = 22; | ||
| root.get<PCSX::ISO9660LowLevel::DirEntry_LBABE>().value = 22; | ||
| root.get<PCSX::ISO9660LowLevel::DirEntry_Size>().value = 2048; | ||
| root.get<PCSX::ISO9660LowLevel::DirEntry_SizeBE>().value = 2048; | ||
| root.get<PCSX::ISO9660LowLevel::DirEntry_Flags>().value = 2; | ||
| root.get<PCSX::ISO9660LowLevel::DirEntry_VolSeqNo>().value = 1; | ||
| root.get<PCSX::ISO9660LowLevel::DirEntry_VolSeqNoBE>().value = 1; | ||
| root.get<PCSX::ISO9660LowLevel::DirEntry_Filename>().value.resize(1); | ||
| auto volumeSetIdent = pvdData["volume_set_id"].is_string() ? pvdData["volume_set_id"].get<std::string>() : ""; | ||
| pvd.get<PCSX::ISO9660LowLevel::PVD_VolSetIdent>().set(volumeSetIdent, ' '); | ||
| auto publisherIdent = pvdData["publisher"].is_string() ? pvdData["publisher"].get<std::string>() : ""; | ||
| pvd.get<PCSX::ISO9660LowLevel::PVD_PublisherIdent>().set(publisherIdent, ' '); | ||
| auto dataPreparerIdent = pvdData["preparer"].is_string() ? pvdData["preparer"].get<std::string>() : ""; | ||
| pvd.get<PCSX::ISO9660LowLevel::PVD_DataPreparerIdent>().set(dataPreparerIdent, ' '); | ||
| auto applicationIdent = pvdData["application_id"].is_string() ? pvdData["application_id"].get<std::string>() : ""; | ||
| pvd.get<PCSX::ISO9660LowLevel::PVD_ApplicationIdent>().set(applicationIdent, ' '); | ||
| auto copyrightFileIdent = pvdData["copyright"].is_string() ? pvdData["copyright"].get<std::string>() : ""; | ||
| pvd.get<PCSX::ISO9660LowLevel::PVD_CopyrightFileIdent>().set(copyrightFileIdent, ' '); | ||
| auto abstractFileIdent = pvdData["abstract"].is_string() ? pvdData["abstract"].get<std::string>() : ""; | ||
| pvd.get<PCSX::ISO9660LowLevel::PVD_AbstractFileIdent>().set(abstractFileIdent, ' '); | ||
| auto bibliographicFileIdent = | ||
| pvdData["bibliographic"].is_string() ? pvdData["bibliographic"].get<std::string>() : ""; | ||
| pvd.get<PCSX::ISO9660LowLevel::PVD_BibliographicFileIdent>().set(bibliographicFileIdent, ' '); | ||
| pvd.get<PCSX::ISO9660LowLevel::PVD_FileStructureVersion>().value = 1; | ||
|
|
||
| pvd.serialize(pvdSector); | ||
| while (pvdSector->size() < 2048) { | ||
| pvdSector->write<uint8_t>(0); | ||
| } | ||
| builder.writeSectorAt(pvdSector.asA<PCSX::BufferFile>()->borrow(0).data<uint8_t>(), {0, 2, 16}, | ||
| PCSX::IEC60908b::SectorMode::M2_FORM1); | ||
|
|
||
| uint8_t sector[2048]; | ||
| memset(sector, 0, sizeof(sector)); | ||
| sector[0] = 0xff; | ||
| sector[1] = 'C'; | ||
| sector[2] = 'D'; | ||
| sector[3] = '0'; | ||
| sector[4] = '0'; | ||
| sector[5] = '1'; | ||
| builder.writeSectorAt(sector, {0, 2, 17}, PCSX::IEC60908b::SectorMode::M2_FORM1); | ||
|
|
||
| memset(sector, 0, sizeof(sector)); | ||
| sector[0] = 1; | ||
| sector[2] = 22; | ||
| sector[6] = 1; | ||
| builder.writeSectorAt(sector, {0, 2, 18}, PCSX::IEC60908b::SectorMode::M2_FORM1); | ||
| builder.writeSectorAt(sector, {0, 2, 19}, PCSX::IEC60908b::SectorMode::M2_FORM1); | ||
|
|
||
| memset(sector, 0, sizeof(sector)); | ||
| sector[0] = 1; | ||
| sector[5] = 22; | ||
| sector[7] = 1; | ||
| builder.writeSectorAt(sector, {0, 2, 20}, PCSX::IEC60908b::SectorMode::M2_FORM1); | ||
| builder.writeSectorAt(sector, {0, 2, 21}, PCSX::IEC60908b::SectorMode::M2_FORM1); | ||
|
|
||
| uint8_t rootSector[2048] = { | ||
| 0x22, 0x00, 0x16, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x16, 0x00, 0x08, 0x00, 0x00, 0x00, 0x00, | ||
| 0x08, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x02, 0x00, 0x00, 0x01, 0x00, 0x00, 0x01, | ||
| 0x01, 0x00, 0x22, 0x00, 0x16, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x16, 0x00, 0x08, 0x00, 0x00, | ||
| 0x00, 0x00, 0x08, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x02, 0x00, 0x00, 0x01, 0x00, | ||
| 0x00, 0x01, 0x01, 0x01, 0x2a, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, | ||
| 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, | ||
| 0x01, 0x00, 0x00, 0x01, 0x09, 0x50, 0x53, 0x58, 0x2e, 0x45, 0x58, 0x45, 0x3b, 0x31, | ||
| }; | ||
| writeToBuffer<uint32_t, std::endian::little>(rootSector + 70, indexSectorsCount + 23); | ||
| writeToBuffer<uint32_t, std::endian::big>(rootSector + 74, indexSectorsCount + 23); | ||
| writeToBuffer<uint32_t, std::endian::little>(rootSector + 78, executableSectorsCount * 2048); | ||
| writeToBuffer<uint32_t, std::endian::big>(rootSector + 82, executableSectorsCount * 2048); | ||
| builder.writeSectorAt(rootSector, {0, 2, 22}, PCSX::IEC60908b::SectorMode::M2_FORM1); | ||
|
|
||
| return 0; | ||
| } |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Main function is too large and complex.
This function has a cyclomatic complexity of 53 (threshold is 9) with deeply nested conditional logic, making it difficult to maintain and test. Consider refactoring it into smaller, more focused functions.
Here are suggested modular functions to extract:
parseCommandLineArgs()- Handle argument parsing and validationloadAndValidateJsonInput()- Load and validate the JSON structuresetupOutputAndLicenseFiles()- Open output file and optional license fileprocessExecutable()- Handle executable loading and processingcompressFiles()- Set up threading and compress fileswriteSectors()- Write all sectors to the output filecreateISOStructure()- Create ISO9660 structure
Each of these would have clear inputs, outputs, and error handling, making the code more maintainable and testable.
🏁 Script executed:
#!/bin/bash
# Count the number of return statements in main function as an indicator of complexity
echo "Main function complexity indicators:"
# Count conditional statements
echo "Number of if statements: $(grep -c "if\s*(" tools/authoring/authoring.cc)"
# Count for loops
echo "Number of for loops: $(grep -c "for\s*(" tools/authoring/authoring.cc)"
# Count while loops
echo "Number of while loops: $(grep -c "while\s*(" tools/authoring/authoring.cc)"
# Count early returns
echo "Number of return statements: $(grep -c "return -1;" tools/authoring/authoring.cc)"Length of output: 538
Refactor the Main Function into Smaller, Modular Components
The verification confirms that the main function in tools/authoring/authoring.cc is excessively complex. Based on the metrics, there are 25 if statements, 9 for loops, 2 while loops, and 17 early return -1; statements, which align with the high cyclomatic complexity originally noted. This level of complexity makes the code hard to maintain and test.
Please consider refactoring the main function into smaller, focused functions with clear responsibilities. For example:
parseCommandLineArgs()– Handle all argument parsing and validation.loadAndValidateJsonInput()– Load and validate the JSON structure.setupOutputAndLicenseFiles()– Open the output file and optionally, the license file.processExecutable()– Load and process the executable file.compressFiles()– Manage threading and file compression.writeSectors()– Write all sectors to the output file.createISOStructure()– Build the ISO9660 structure.
Breaking down the current monolithic function into these parts would drastically improve maintainability and testability.
🧰 Tools
🪛 GitHub Check: CodeScene Cloud Delta Analysis (main)
[warning] 93-493: ❌ New issue: Complex Method
main has a cyclomatic complexity of 53, threshold = 9. 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] 102-102: ❌ New issue: Complex Conditional
main has 1 complex conditionals with 2 branches, threshold = 2. A complex conditional is an expression inside a branch (e.g. if, for, while) which consists of multiple, logical operators such as AND/OR. The more logical operators in an expression, the more severe the code smell.
[warning] 93-493: ❌ New issue: Bumpy Road Ahead
main has 5 blocks with nested conditional logic. Any nesting of 2 or deeper is considered. Threshold is one single, nested block per function. The Bumpy Road code smell is a function that contains multiple chunks of nested conditional logic. The deeper the nesting and the more bumps, the lower the code health.
[warning] 93-493: ❌ New issue: Deep, Nested Complexity
main has a nested complexity depth of 4, threshold = 4. This function contains deeply nested logic such as if statements and/or loops. The deeper the nesting, the lower the code health.
nicolasnoble
force-pushed
the
authoring
branch
2 times, most recently
from
82ad17a to
970d424
Compare
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
Actionable comments posted: 2
♻️ Duplicate comments (1)
tools/authoring/authoring.cc (1)
93-496:⚠️ Potential issueMain function requires significant refactoring to reduce complexity.
The function has a cyclomatic complexity of 53 (threshold is 9) with deeply nested conditional logic, making it difficult to maintain and test.
Consider refactoring into smaller, focused functions such as:
parseCommandLineArgs()- Handle argument parsing and validationloadAndValidateJsonInput()- Load and validate the JSON structuresetupOutputAndLicenseFiles()- Open output file and optional license fileprocessExecutable()- Handle executable loading and processingcompressFiles()- Set up threading and compress fileswriteSectors()- Write all sectors to the output filecreateISOStructure()- Create ISO9660 structureEach of these would have clear inputs, outputs, and error handling, making the code more maintainable and testable.
🧰 Tools
🪛 GitHub Check: CodeScene Cloud Delta Analysis (main)
[warning] 93-496: ❌ New issue: Complex Method
main has a cyclomatic complexity of 53, threshold = 9. 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] 102-102: ❌ New issue: Complex Conditional
main has 1 complex conditionals with 2 branches, threshold = 2. A complex conditional is an expression inside a branch (e.g. if, for, while) which consists of multiple, logical operators such as AND/OR. The more logical operators in an expression, the more severe the code smell.
[warning] 93-496: ❌ New issue: Bumpy Road Ahead
main has 5 blocks with nested conditional logic. Any nesting of 2 or deeper is considered. Threshold is one single, nested block per function. The Bumpy Road code smell is a function that contains multiple chunks of nested conditional logic. The deeper the nesting and the more bumps, the lower the code health.
[warning] 93-496: ❌ New issue: Deep, Nested Complexity
main has a nested complexity depth of 4, threshold = 4. This function contains deeply nested logic such as if statements and/or loops. The deeper the nesting, the lower the code health.
🧹 Nitpick comments (4)
tools/exe2iso/exe2iso.cc (1)
90-237: Consider breaking down the main function to reduce complexityThe static analysis tool indicates that the
mainfunction has high cyclomatic complexity (many conditional branches) and is getting more complex with these changes. While the current changes are necessary for correct functionality, consider refactoring this function into smaller, more focused functions in a future update.For example, you could extract license handling into a separate function:
+ static bool handleLicenseFile(PCSX::IO<PCSX::File>& licenseFile, PCSX::IO<PCSX::File>& out, + uint8_t sector[2352], unsigned& LBA, bool regen) { + if (licenseFile && !licenseFile->failed()) { + uint8_t licenseData[2352 * 16]; + memset(licenseData, 0, sizeof(licenseData)); + licenseFile->read(licenseData, sizeof(licenseData)); + // License handling logic... + return true; + } + return false; + }Similar extractions could be done for data file handling, sector writing logic, etc.
🧰 Tools
🪛 GitHub Check: CodeScene Cloud Delta Analysis (main)
[warning] 98-194: ❌ Getting worse: Complex Method
main already has high cyclomatic complexity, and now it increases in Lines of Code from 131 to 132. 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.tools/authoring/authoring.cc (1)
238-257: Using a fixed-size static array for work units.While a check was added to prevent overflow, consider using a dynamically sized container like
std::vectorinstead of a fixed-size array for better flexibility and safety.-static WorkUnit work[c_maximumSectorCount]; +std::vector<WorkUnit> work(filesCount);tools/authoring/README.md (2)
35-38: Minor grammar issues in documentation.There are a few grammatical improvements needed in this section.
-All the file lookups will be relative to base directory, which is the same as the json file if the option `-basedir` isn't specified. The files will be stored into the archive in their order of appearance. +All the file lookups will be relative to the base directory, which is the same as the json file if the option `-basedir` isn't specified. The files will be stored in the archive in their order of appearance.🧰 Tools
🪛 LanguageTool
[uncategorized] ~37-~37: You might be missing the article “the” here.
Context: ...ll the file lookups will be relative to base directory, which is the same as the jso...(AI_EN_LECTOR_MISSING_DETERMINER_THE)
[uncategorized] ~37-~37: The preposition “in” seems more likely in this position than the preposition “into”.
Context: ...n't specified. The files will be stored into the archive in their order of appearanc...(AI_EN_LECTOR_REPLACEMENT_PREPOSITION_INTO_IN)
39-45: Command line arguments section needs formatting improvement.The command line arguments section uses markdown list formatting, but the static analysis suggests there might be loose punctuation marks.
Consider using a different markdown list format or ensuring proper spacing:
-The tool accepts the following command line arguments: -- `-o`: The output file name for the CD image. Mandatory. -- `-license`: The license file to be used. Optional. Point to an existing valid PlayStation 1 disc, or an official license file. The tool will use the license file to generate the CD image. If not provided, the tool will generate an empty license on the CD image. -- `-basedir`: The base directory for the files. Optional. Default is the directory where the json file is located. -- `-threads`: The number of threads to use for compression. Optional. Default is the number of available CPU cores. +The tool accepts the following command line arguments: + +* `-o`: The output file name for the CD image. Mandatory. +* `-license`: The license file to be used. Optional. Point to an existing valid PlayStation 1 disc, or an official license file. The tool will use the license file to generate the CD image. If not provided, the tool will generate an empty license on the CD image. +* `-basedir`: The base directory for the files. Optional. Default is the directory where the json file is located. +* `-threads`: The number of threads to use for compression. Optional. Default is the number of available CPU cores.🧰 Tools
🪛 LanguageTool
[uncategorized] ~41-~41: Loose punctuation mark.
Context: ...ollowing command line arguments: --o: The output file name for the CD image. ...(UNLIKELY_OPENING_PUNCTUATION)
[uncategorized] ~42-~42: Loose punctuation mark.
Context: ...or the CD image. Mandatory. --license: The license file to be used. Optional. ...(UNLIKELY_OPENING_PUNCTUATION)
[uncategorized] ~43-~43: Loose punctuation mark.
Context: ...ty license on the CD image. --basedir: The base directory for the files. Optio...(UNLIKELY_OPENING_PUNCTUATION)
[uncategorized] ~44-~44: Loose punctuation mark.
Context: ...e the json file is located. --threads: The number of threads to use for compre...(UNLIKELY_OPENING_PUNCTUATION)
🛑 Comments failed to post (2)
src/support/file.h (1)
377-377: 💡 Verification agent
🧩 Analysis chain
Verify all callers support the updated method signature
The
borrowmethod signature has been updated to include an optional offset parameter. This is a backward-compatible change since the parameter is defaulted to 0, but ensure all callers are updated if they need to use this new functionality.
🏁 Script executed:
#!/bin/bash # Search for all callers of the borrow method to verify compatibility rg -A 2 "->borrow\(" --type cpp rg -A 2 "\.borrow\(" --type cppLength of output: 4309
Attention: Misaligned
borrow()Calls Require UpdateThe updated definition in
src/support/file.hnow declares:Slice borrow(size_t offset = 0);However, our search indicates that many call sites are still invoking
borrow()with two arguments (e.g., insrc/support/file.cc,src/gpu/soft/interface.h,src/core/web-server.cc, etc.). This discrepancy needs to be addressed—either update the call sites to match the new single-argument (offset-only) signature or provide an appropriate overload to support the two-argument invocations.
- src/support/file.cc: Call like
ret.borrow(m_data + offset, m_size - offset);- src/gpu/soft/interface.h: Call like
ret.borrow(m_vram16, 1024 * 512 * 2);- src/gui/widgets/typed_debugger.cc and several others also pass two arguments.
Please review these locations and update them accordingly so that they either use the new functionality (with the single optional offset) or maintain proper backward compatibility through an overloaded method.
tools/authoring/authoring.cc (1)
264-339:
⚠️ Potential issueThread management needs improvement.
Threads are created and detached without proper synchronization for termination. This could lead to resource leaks or undefined behavior if the program terminates before all threads complete.
Consider using a thread pool or
std::jthread(C++20) to manage thread lifecycles, or explicitly join threads before program exit.-std::thread t([&]() { - // Thread work... -}); -t.detach(); +std::vector<std::thread> threads; +for (unsigned i = 0; i < threadCount; i++) { + threads.emplace_back([&]() { + // Thread work... + }); +} + +// Join all threads before continuing +for (auto& t : threads) { + t.join(); +}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.std::atomic<unsigned> currentWorkUnit = 0; std::vector<std::thread> threads; for (unsigned i = 0; i < threadCount; i++) { threads.emplace_back([&]() { while (1) { std::atomic_thread_fence(std::memory_order_acq_rel); unsigned workUnitIndex = currentWorkUnit.fetch_add(1); if (workUnitIndex >= filesCount) return; auto& workUnit = work[workUnitIndex]; auto filePath = workUnit.fileInfo["path"].get<std::string>(); PCSX::IO<PCSX::File> file(new PCSX::PosixFile(basePath / filePath)); if (file->failed()) { workUnit.failed = true; workUnit.semaphore.release(); continue; } unsigned size = file->size(); unsigned originalSectorsCount = (size + 2047) / 2048; std::vector<uint8_t> dataIn; dataIn.resize(originalSectorsCount * 2048); file->read(dataIn.data(), dataIn.size()); std::vector<uint8_t> dataOut; dataOut.resize(dataIn.size() * 1.2 + 2064 + 2048); ucl_uint outSize; int r; r = ucl_nrv2e_99_compress(dataIn.data(), size, dataOut.data() + 2048, &outSize, nullptr, 10, nullptr, nullptr); if (r != UCL_E_OK) { workUnit.failed = true; workUnit.semaphore.release(); continue; } unsigned compressedSectorsCount = (outSize + 2047) / 2048; IndexEntry* entry = &indexEntryData[workUnitIndex]; if (workUnit.fileInfo["name"].is_string()) { entry->hash = PCSX::djb::hash(workUnit.fileInfo["name"].get<std::string>()); } else { entry->hash = PCSX::djb::hash(filePath); } entry->setDecompSize(size); std::span<uint8_t> source; unsigned sectorCount = 0; if (compressedSectorsCount < originalSectorsCount) { entry->setCompressedSize(compressedSectorsCount); entry->setMethod(IndexEntry::Method::UCL_NRV2E); workUnit.sectorData.resize(compressedSectorsCount * 2352); unsigned padding = outSize % 2048; if (padding > 0) { padding = 2048 - padding; } entry->setPadding(padding); sectorCount = compressedSectorsCount; source = {reinterpret_cast<uint8_t*>(dataOut.data()) - padding, sectorCount * 2048}; } else { entry->setCompressedSize(originalSectorsCount); entry->setMethod(IndexEntry::Method::NONE); workUnit.sectorData.resize(originalSectorsCount * 2352); entry->setPadding(0); sectorCount = originalSectorsCount; source = {reinterpret_cast<uint8_t*>(dataIn.data()), sectorCount * 2048}; } for (unsigned sector = 0; sector < sectorCount; sector++) { uint8_t* dest = workUnit.sectorData.data() + sector * 2352; createSectorHeader(dest); memcpy(dest + 24, source.data() + sector * 2048, 2048); PCSX::IEC60908b::computeEDCECC(dest); } workUnit.semaphore.release(); } }); } // Join all threads before continuing for (auto& t : threads) { t.join(); }
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
tools/authoring/authoring.cc (1)
238-257: Consider using a dynamic container instead of static array.While you've added a check to prevent overflow, using a dynamic container like
std::vector<WorkUnit>would be more flexible and safer than a static array with a large maximum size.-static WorkUnit work[c_maximumSectorCount]; +std::vector<WorkUnit> work(filesCount);tools/authoring/README.md (1)
1-44: Well-structured documentation for the CD Authoring tool.The README provides clear documentation about the tool's purpose, usage, and expected input format. The JSON example and command-line options are well explained.
Consider these minor improvements for clarity:
- Line 5: Change "only requires to have" to "only requires having"
- Line 37: Add "the" before "base directory"
- Line 37: Change "stored into" to "stored in"
🧰 Tools
🪛 LanguageTool
[grammar] ~5-~5: Did you mean “having”? Or maybe you should add a pronoun? In active voice, ‘require’ + ‘to’ takes an object, usually a pronoun.
Context: ...means the archive manager only requires to have the UCL-NRV2E decompressor registered. ...(ALLOW_TO)
[uncategorized] ~37-~37: You might be missing the article “the” here.
Context: ...ll the file lookups will be relative to base directory, which is the same as the jso...(AI_EN_LECTOR_MISSING_DETERMINER_THE)
[uncategorized] ~37-~37: The preposition “in” seems more likely in this position than the preposition “into”.
Context: ...n't specified. The files will be stored into the archive in their order of appearanc...(AI_EN_LECTOR_REPLACEMENT_PREPOSITION_INTO_IN)
[uncategorized] ~41-~41: Loose punctuation mark.
Context: ...ollowing command line arguments: --o: The output file name for the CD image. ...(UNLIKELY_OPENING_PUNCTUATION)
[uncategorized] ~42-~42: Loose punctuation mark.
Context: ...or the CD image. Mandatory. --license: The license file to be used. Optional. ...(UNLIKELY_OPENING_PUNCTUATION)
[uncategorized] ~43-~43: Loose punctuation mark.
Context: ...ty license on the CD image. --basedir: The base directory for the files. Optio...(UNLIKELY_OPENING_PUNCTUATION)
[uncategorized] ~44-~44: Loose punctuation mark.
Context: ...e the json file is located. --threads: The number of threads to use for compre...(UNLIKELY_OPENING_PUNCTUATION)
tools/exe2iso/exe2iso.cc (1)
90-237: Consider refactoring the main function to reduce complexityWhile the current changes are necessary for correctness and don't significantly increase complexity, the static analysis tools flag that the
mainfunction already has high cyclomatic complexity with many conditional statements. Consider extracting some logic into helper functions to improve maintainability.Potential refactoring opportunities:
- Extract license handling logic (lines 169-198) into a separate function
- Extract data writing logic (lines 216-227) into a separate function
- Extract padding logic (lines 230-235) into a separate function
🧰 Tools
🪛 GitHub Check: CodeScene Cloud Delta Analysis (main)
[warning] 98-194: ❌ Getting worse: Complex Method
main already has high cyclomatic complexity, and now it increases in Lines of Code from 131 to 132. 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 (35)
.github/filter-support/Makefile-filtered(1 hunks).gitignore(1 hunks)Makefile(1 hunks)azure-pipelines-cli.yml(2 hunks)azure-pipelines.yml(2 hunks)src/cdrom/common.h(0 hunks)src/cdrom/file.cc(5 hunks)src/cdrom/file.h(3 hunks)src/cdrom/iso9660-builder.h(0 hunks)src/cdrom/iso9660-reader.cc(1 hunks)src/cdrom/iso9660-reader.h(1 hunks)src/core/luaiso.cc(3 hunks)src/core/web-server.cc(3 hunks)src/mips/common/util/util.h(1 hunks)src/support/binstruct.h(1 hunks)src/support/file.cc(2 hunks)src/support/file.h(3 hunks)src/supportpsx/binffi.lua(1 hunks)src/supportpsx/iec-60908b.h(1 hunks)src/supportpsx/iso9660-builder.cc(4 hunks)src/supportpsx/iso9660-builder.h(1 hunks)src/supportpsx/iso9660-lowlevel.h(3 hunks)tools/authoring/README.md(1 hunks)tools/authoring/authoring.cc(1 hunks)tools/exe2iso/exe2iso.cc(2 hunks)tools/modconv/modconv.cc(2 hunks)tools/ps1-packer/ps1-packer.cc(2 hunks)vsprojects/authoring/authoring.vcxproj(1 hunks)vsprojects/authoring/authoring.vcxproj.filters(1 hunks)vsprojects/cdrom/cdrom.vcxproj(0 hunks)vsprojects/cdrom/cdrom.vcxproj.filters(0 hunks)vsprojects/exe2iso/exe2iso.vcxproj(1 hunks)vsprojects/pcsx-redux.sln(3 hunks)vsprojects/supportpsx/supportpsx.vcxproj(2 hunks)vsprojects/supportpsx/supportpsx.vcxproj.filters(2 hunks)
💤 Files with no reviewable changes (4)
- src/cdrom/common.h
- src/cdrom/iso9660-builder.h
- vsprojects/cdrom/cdrom.vcxproj
- vsprojects/cdrom/cdrom.vcxproj.filters
🚧 Files skipped from review as they are similar to previous changes (24)
- .gitignore
- src/cdrom/iso9660-reader.h
- azure-pipelines.yml
- src/mips/common/util/util.h
- azure-pipelines-cli.yml
- src/supportpsx/iec-60908b.h
- vsprojects/exe2iso/exe2iso.vcxproj
- src/cdrom/iso9660-reader.cc
- .github/filter-support/Makefile-filtered
- src/supportpsx/binffi.lua
- src/support/binstruct.h
- Makefile
- vsprojects/supportpsx/supportpsx.vcxproj.filters
- tools/ps1-packer/ps1-packer.cc
- vsprojects/authoring/authoring.vcxproj.filters
- src/core/luaiso.cc
- vsprojects/authoring/authoring.vcxproj
- tools/modconv/modconv.cc
- src/cdrom/file.h
- vsprojects/supportpsx/supportpsx.vcxproj
- src/cdrom/file.cc
- src/core/web-server.cc
- src/supportpsx/iso9660-builder.h
- src/supportpsx/iso9660-lowlevel.h
🧰 Additional context used
🪛 GitHub Check: CodeScene Cloud Delta Analysis (main)
tools/authoring/authoring.cc
[warning] 93-496: ❌ New issue: Complex Method
main has a cyclomatic complexity of 53, threshold = 9. 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] 102-102: ❌ New issue: Complex Conditional
main has 1 complex conditionals with 2 branches, threshold = 2. A complex conditional is an expression inside a branch (e.g. if, for, while) which consists of multiple, logical operators such as AND/OR. The more logical operators in an expression, the more severe the code smell.
[warning] 93-496: ❌ New issue: Bumpy Road Ahead
main has 5 blocks with nested conditional logic. Any nesting of 2 or deeper is considered. Threshold is one single, nested block per function. The Bumpy Road code smell is a function that contains multiple chunks of nested conditional logic. The deeper the nesting and the more bumps, the lower the code health.
[warning] 93-496: ❌ New issue: Deep, Nested Complexity
main has a nested complexity depth of 4, threshold = 4. This function contains deeply nested logic such as if statements and/or loops. The deeper the nesting, the lower the code health.
tools/exe2iso/exe2iso.cc
[warning] 98-194: ❌ Getting worse: Complex Method
main already has high cyclomatic complexity, and now it increases in Lines of Code from 131 to 132. 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] 98-194: ❌ Getting worse: Complex Method
main already has high cyclomatic complexity, and now it increases in Lines of Code from 131 to 132. 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.
🪛 LanguageTool
tools/authoring/README.md
[grammar] ~5-~5: Did you mean “having”? Or maybe you should add a pronoun? In active voice, ‘require’ + ‘to’ takes an object, usually a pronoun.
Context: ...means the archive manager only requires to have the UCL-NRV2E decompressor registered. ...
(ALLOW_TO)
[uncategorized] ~37-~37: You might be missing the article “the” here.
Context: ...ll the file lookups will be relative to base directory, which is the same as the jso...
(AI_EN_LECTOR_MISSING_DETERMINER_THE)
[uncategorized] ~37-~37: The preposition “in” seems more likely in this position than the preposition “into”.
Context: ...n't specified. The files will be stored into the archive in their order of appearanc...
(AI_EN_LECTOR_REPLACEMENT_PREPOSITION_INTO_IN)
[uncategorized] ~41-~41: Loose punctuation mark.
Context: ...ollowing command line arguments: - -o: The output file name for the CD image. ...
(UNLIKELY_OPENING_PUNCTUATION)
[uncategorized] ~42-~42: Loose punctuation mark.
Context: ...or the CD image. Mandatory. - -license: The license file to be used. Optional. ...
(UNLIKELY_OPENING_PUNCTUATION)
[uncategorized] ~43-~43: Loose punctuation mark.
Context: ...ty license on the CD image. - -basedir: The base directory for the files. Optio...
(UNLIKELY_OPENING_PUNCTUATION)
[uncategorized] ~44-~44: Loose punctuation mark.
Context: ...e the json file is located. - -threads: The number of threads to use for compre...
(UNLIKELY_OPENING_PUNCTUATION)
⏰ Context from checks skipped due to timeout of 90000ms (5)
- GitHub Check: Codacy Static Code Analysis
- GitHub Check: cross-arm64
- GitHub Check: build
- GitHub Check: coverage
- GitHub Check: aur-build
🔇 Additional comments (21)
vsprojects/pcsx-redux.sln (4)
112-113: New CD authoring tool project addedThe new "authoring" project has been properly added to the solution file with a unique GUID, matching the PR objective of adding a CD authoring tool.
523-532: Build configurations properly configured for the authoring toolAll standard build configurations (Debug, Release, ReleaseCLI, ReleaseWithClangCL) have been correctly set up for the new authoring tool project. The project is properly configured to build across different configurations.
Note that for ReleaseWithTracy, the project builds with the Release configuration rather than a Tracy-specific configuration. This is consistent with other tools in the solution.
580-580: Project correctly organized in solution structureThe new authoring tool project has been appropriately nested under the "tools" folder in the solution, maintaining consistent organization with other utility tools.
584-584: Solution GUID has been updatedThe SolutionGuid has changed from
{284E91C1-764E-441A-854D-CFD3623A6501}to{AC54A867-F976-4B3D-A6EF-F57EB764DCD4}. This is expected when adding a new project to the solution.src/support/file.h (3)
344-344: New Borrow enumeration added for memory management.This new enumeration complements the existing
Acquireenum, providing a clear way to indicate when memory should be borrowed rather than acquired or copied.
356-359: New constructor enables memory borrowing.The added constructor allows for creating a
BufferFilethat borrows memory without taking ownership. This provides more flexibility in memory management, particularly when working with pre-allocated buffers that have their own lifecycle management.
377-377: Enhanced borrow method with optional offset parameter.The addition of an optional offset parameter to the
borrow()method provides more flexibility when creating Slices, allowing access to specific portions of the buffer without needing to create intermediate buffers.src/support/file.cc (3)
55-59: Implementation of the BufferFile Borrow constructor.The implementation correctly initializes the buffer with borrowed memory by setting the data pointer, size, and marking it as not owned (
m_owned = false).
155-155: Improved dup() method with ownership check.The addition of the ownership check (
m_owned) ensures that the duplication behavior is correctly handled based on whether the BufferFile owns its memory.
162-164: Updated borrow() implementation to support offset parameter.The implementation correctly applies the offset to the data pointer and adjusts the size accordingly when creating the Slice.
src/supportpsx/iso9660-builder.cc (3)
1-25: License updated from GNU to MIT.The license has been changed from GNU to MIT, which is a significant change in terms of licensing terms. This appears to be part of a broader licensing update across the codebase.
27-27: Updated include path for restructured header.The include path has been updated to reflect changes in the code organization.
42-43: Namespace qualification for SectorMode enum.All references to
SectorModehave been updated to use the fully qualified nameIEC60908b::SectorMode. This change improves code organization by properly namespacing the enum.Also applies to: 48-50, 57-58, 61-62, 69-70, 72-72, 81-81, 95-95
tools/exe2iso/exe2iso.cc (4)
98-99: Improved variable safety withconst autodeclarations.Making these variables immutable after initialization is a good practice since they're only used for reading and never modified. This helps prevent accidental modification and better expresses your intent.
🧰 Tools
🪛 GitHub Check: CodeScene Cloud Delta Analysis (main)
[warning] 98-194: ❌ Getting worse: Complex Method
main already has high cyclomatic complexity, and now it increases in Lines of Code from 131 to 132. 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.
194-194: Fixed potential header initialization issue.Adding
makeHeaderOnce(sector)ensures the sector header is properly initialized even when no license is written. This prevents potential undefined behavior when writing sectors.🧰 Tools
🪛 GitHub Check: CodeScene Cloud Delta Analysis (main)
[warning] 98-194: ❌ Getting worse: Complex Method
main already has high cyclomatic complexity, and now it increases in Lines of Code from 131 to 132. 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.
98-99: Improved variable immutabilityThe change from
autotoconst autofor theoutputandinputsvariables is a good practice as it enforces immutability, preventing accidental modifications after initialization and clearly communicating intent to other developers.🧰 Tools
🪛 GitHub Check: CodeScene Cloud Delta Analysis (main)
[warning] 98-194: ❌ Getting worse: Complex Method
main already has high cyclomatic complexity, and now it increases in Lines of Code from 131 to 132. 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.
194-194: Fixed potential header initialization issueAdding
makeHeaderOnce(sector)ensures the sector header is properly initialized even when no license is written. This prevents potential undefined behavior when writing sectors.🧰 Tools
🪛 GitHub Check: CodeScene Cloud Delta Analysis (main)
[warning] 98-194: ❌ Getting worse: Complex Method
main already has high cyclomatic complexity, and now it increases in Lines of Code from 131 to 132. 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.tools/authoring/authoring.cc (4)
46-56: Well-implemented buffer writing utility with endianness handling.This template function properly handles writing values to buffers with correct endianness conversion, making it robust across different platforms.
60-89: Good use of bit fields for compact data representation.The
IndexEntryunion efficiently packs file metadata into 16 bytes using bit fields, with clean accessor methods. The static_assert ensures the size is what you expect.
198-201: Good validation of file count against the static array limit.This check prevents potential buffer overflow by validating that the number of files doesn't exceed the maximum sector count before allocating work units.
265-339: Good use of multithreading for compression.The implementation efficiently uses multiple threads for compression with proper synchronization using semaphores. The thread creation and work distribution is well structured.
tools/authoring/authoring.cc
Outdated
| PCSX::IO<PCSX::File> pvdSector(new PCSX::BufferFile(PCSX::FileOps::READWRITE)); | ||
| PCSX::ISO9660LowLevel::PVD pvd; | ||
| pvd.reset(); | ||
| pvd.get<PCSX::ISO9660LowLevel::PVD_TypeCode>().value = 1; | ||
| pvd.get<PCSX::ISO9660LowLevel::PVD_StdIdent>().set("CD001"); | ||
| pvd.get<PCSX::ISO9660LowLevel::PVD_Version>().value = 1; | ||
| auto systemIdent = pvdData["system_id"].is_string() ? pvdData["system_id"].get<std::string>() : "PLAYSTATION"; | ||
| pvd.get<PCSX::ISO9660LowLevel::PVD_SystemIdent>().set(systemIdent, ' '); | ||
| auto volumeIdent = pvdData["volume_id"].is_string() ? pvdData["volume_id"].get<std::string>() : ""; | ||
| pvd.get<PCSX::ISO9660LowLevel::PVD_VolumeIdent>().set(volumeIdent, ' '); | ||
| pvd.get<PCSX::ISO9660LowLevel::PVD_VolumeSpaceSize>().value = totalSectorCount; | ||
| pvd.get<PCSX::ISO9660LowLevel::PVD_VolumeSpaceSizeBE>().value = totalSectorCount; | ||
| pvd.get<PCSX::ISO9660LowLevel::PVD_VolumeSetSize>().value = 1; | ||
| pvd.get<PCSX::ISO9660LowLevel::PVD_VolumeSetSizeBE>().value = 1; | ||
| pvd.get<PCSX::ISO9660LowLevel::PVD_VolumeSequenceNumber>().value = 1; | ||
| pvd.get<PCSX::ISO9660LowLevel::PVD_VolumeSequenceNumberBE>().value = 1; | ||
| pvd.get<PCSX::ISO9660LowLevel::PVD_LogicalBlockSize>().value = 2048; | ||
| pvd.get<PCSX::ISO9660LowLevel::PVD_LogicalBlockSizeBE>().value = 2048; | ||
| pvd.get<PCSX::ISO9660LowLevel::PVD_PathTableSize>().value = 10; | ||
| pvd.get<PCSX::ISO9660LowLevel::PVD_PathTableSizeBE>().value = 10; | ||
| pvd.get<PCSX::ISO9660LowLevel::PVD_LPathTableLocation>().value = 18; | ||
| pvd.get<PCSX::ISO9660LowLevel::PVD_LPathTableOptLocation>().value = 19; | ||
| pvd.get<PCSX::ISO9660LowLevel::PVD_MPathTableLocation>().value = 20; | ||
| pvd.get<PCSX::ISO9660LowLevel::PVD_MPathTableOptLocation>().value = 21; | ||
| auto& root = pvd.get<PCSX::ISO9660LowLevel::PVD_RootDir>(); | ||
| root.get<PCSX::ISO9660LowLevel::DirEntry_Length>().value = 34; | ||
| root.get<PCSX::ISO9660LowLevel::DirEntry_ExtLength>().value = 0; | ||
| root.get<PCSX::ISO9660LowLevel::DirEntry_LBA>().value = 22; | ||
| root.get<PCSX::ISO9660LowLevel::DirEntry_LBABE>().value = 22; | ||
| root.get<PCSX::ISO9660LowLevel::DirEntry_Size>().value = 2048; | ||
| root.get<PCSX::ISO9660LowLevel::DirEntry_SizeBE>().value = 2048; | ||
| root.get<PCSX::ISO9660LowLevel::DirEntry_Flags>().value = 2; | ||
| root.get<PCSX::ISO9660LowLevel::DirEntry_VolSeqNo>().value = 1; | ||
| root.get<PCSX::ISO9660LowLevel::DirEntry_VolSeqNoBE>().value = 1; | ||
| root.get<PCSX::ISO9660LowLevel::DirEntry_Filename>().value.resize(1); | ||
| auto volumeSetIdent = pvdData["volume_set_id"].is_string() ? pvdData["volume_set_id"].get<std::string>() : ""; | ||
| pvd.get<PCSX::ISO9660LowLevel::PVD_VolSetIdent>().set(volumeSetIdent, ' '); | ||
| auto publisherIdent = pvdData["publisher"].is_string() ? pvdData["publisher"].get<std::string>() : ""; | ||
| pvd.get<PCSX::ISO9660LowLevel::PVD_PublisherIdent>().set(publisherIdent, ' '); | ||
| auto dataPreparerIdent = pvdData["preparer"].is_string() ? pvdData["preparer"].get<std::string>() : ""; | ||
| pvd.get<PCSX::ISO9660LowLevel::PVD_DataPreparerIdent>().set(dataPreparerIdent, ' '); | ||
| auto applicationIdent = pvdData["application_id"].is_string() ? pvdData["application_id"].get<std::string>() : ""; | ||
| pvd.get<PCSX::ISO9660LowLevel::PVD_ApplicationIdent>().set(applicationIdent, ' '); | ||
| auto copyrightFileIdent = pvdData["copyright"].is_string() ? pvdData["copyright"].get<std::string>() : ""; | ||
| pvd.get<PCSX::ISO9660LowLevel::PVD_CopyrightFileIdent>().set(copyrightFileIdent, ' '); | ||
| auto abstractFileIdent = pvdData["abstract"].is_string() ? pvdData["abstract"].get<std::string>() : ""; | ||
| pvd.get<PCSX::ISO9660LowLevel::PVD_AbstractFileIdent>().set(abstractFileIdent, ' '); | ||
| auto bibliographicFileIdent = | ||
| pvdData["bibliographic"].is_string() ? pvdData["bibliographic"].get<std::string>() : ""; | ||
| pvd.get<PCSX::ISO9660LowLevel::PVD_BibliographicFileIdent>().set(bibliographicFileIdent, ' '); | ||
| pvd.get<PCSX::ISO9660LowLevel::PVD_FileStructureVersion>().value = 1; | ||
|
|
||
| pvd.serialize(pvdSector); | ||
| while (pvdSector->size() < 2048) { | ||
| pvdSector->write<uint8_t>(0); | ||
| } | ||
| builder.writeSectorAt(pvdSector.asA<PCSX::BufferFile>()->borrow(0).data<uint8_t>(), {0, 2, 16}, | ||
| PCSX::IEC60908b::SectorMode::M2_FORM1); | ||
|
|
||
| uint8_t sector[2048]; | ||
| memset(sector, 0, sizeof(sector)); | ||
| sector[0] = 0xff; | ||
| sector[1] = 'C'; | ||
| sector[2] = 'D'; | ||
| sector[3] = '0'; | ||
| sector[4] = '0'; | ||
| sector[5] = '1'; | ||
| builder.writeSectorAt(sector, {0, 2, 17}, PCSX::IEC60908b::SectorMode::M2_FORM1); | ||
|
|
||
| memset(sector, 0, sizeof(sector)); | ||
| sector[0] = 1; | ||
| sector[2] = 22; | ||
| sector[6] = 1; | ||
| builder.writeSectorAt(sector, {0, 2, 18}, PCSX::IEC60908b::SectorMode::M2_FORM1); | ||
| builder.writeSectorAt(sector, {0, 2, 19}, PCSX::IEC60908b::SectorMode::M2_FORM1); | ||
|
|
||
| memset(sector, 0, sizeof(sector)); | ||
| sector[0] = 1; | ||
| sector[5] = 22; | ||
| sector[7] = 1; | ||
| builder.writeSectorAt(sector, {0, 2, 20}, PCSX::IEC60908b::SectorMode::M2_FORM1); | ||
| builder.writeSectorAt(sector, {0, 2, 21}, PCSX::IEC60908b::SectorMode::M2_FORM1); | ||
|
|
||
| uint8_t rootSector[2048] = { | ||
| 0x22, 0x00, 0x16, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x16, 0x00, 0x08, 0x00, 0x00, 0x00, 0x00, | ||
| 0x08, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x02, 0x00, 0x00, 0x01, 0x00, 0x00, 0x01, | ||
| 0x01, 0x00, 0x22, 0x00, 0x16, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x16, 0x00, 0x08, 0x00, 0x00, | ||
| 0x00, 0x00, 0x08, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x02, 0x00, 0x00, 0x01, 0x00, | ||
| 0x00, 0x01, 0x01, 0x01, 0x2a, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, | ||
| 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, | ||
| 0x01, 0x00, 0x00, 0x01, 0x09, 0x50, 0x53, 0x58, 0x2e, 0x45, 0x58, 0x45, 0x3b, 0x31, | ||
| }; | ||
| writeToBuffer<uint32_t, std::endian::little>(rootSector + 70, indexSectorsCount + 23); | ||
| writeToBuffer<uint32_t, std::endian::big>(rootSector + 74, indexSectorsCount + 23); | ||
| writeToBuffer<uint32_t, std::endian::little>(rootSector + 78, executableSectorsCount * 2048); | ||
| writeToBuffer<uint32_t, std::endian::big>(rootSector + 82, executableSectorsCount * 2048); | ||
| builder.writeSectorAt(rootSector, {0, 2, 22}, PCSX::IEC60908b::SectorMode::M2_FORM1); | ||
|
|
||
| return 0; | ||
| } |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Refactor the main function into smaller, focused functions.
The main function has a cyclomatic complexity of 53 (threshold is 9) with deeply nested conditional logic, making it difficult to maintain and test. Consider breaking it down into smaller, more focused functions.
Suggested structure to refactor into:
parseCommandLineArgs()- Handle argument parsing and validationloadAndValidateJsonInput()- Load and validate the JSON structuresetupOutputAndLicenseFiles()- Open output file and optional license fileprocessExecutable()- Handle executable loading and processingcompressFiles()- Set up threading and compress fileswriteSectors()- Write all sectors to the output filecreateISOStructure()- Create ISO9660 structure
Each would have clear inputs, outputs, and error handling.
🏁 Script executed:
#!/bin/bash
# Count metrics to verify complexity
echo "Main function complexity indicators:"
# Count conditional statements
echo "Number of if statements: $(grep -c "if\s*(" tools/authoring/authoring.cc)"
# Count for loops
echo "Number of for loops: $(grep -c "for\s*(" tools/authoring/authoring.cc)"
# Count while loops
echo "Number of while loops: $(grep -c "while\s*(" tools/authoring/authoring.cc)"
# Count early returns
echo "Number of return statements: $(grep -c "return -1;" tools/authoring/authoring.cc)"Length of output: 538
Refactor main() into Smaller, Focused Functions
The complexity metrics we gathered indicate that main() remains overly complex—with 25 conditional statements, 9 for-loops, 2 while-loops, and 17 early returns—which confirms the challenges in readability, testing, and maintenance.
To improve clarity and maintainability, consider decomposing main() into dedicated functions with clearly defined responsibilities. For example:
parseCommandLineArgs()– Extracts and validates command-line arguments.loadAndValidateJsonInput()– Parses and verifies the JSON input.setupOutputAndLicenseFiles()– Opens and validates the output and optional license files.processExecutable()– Loads and processes the executable file.compressFiles()– Handles multi-threaded file compression.writeSectors()– Manages writing sectors to the output.createISOStructure()– Constructs the ISO9660 structure.
Refactoring along these lines will reduce cyclomatic complexity, simplify error handling, and ease future testing and maintenance.
🧰 Tools
🪛 GitHub Check: CodeScene Cloud Delta Analysis (main)
[warning] 93-496: ❌ New issue: Complex Method
main has a cyclomatic complexity of 53, threshold = 9. 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] 102-102: ❌ New issue: Complex Conditional
main has 1 complex conditionals with 2 branches, threshold = 2. A complex conditional is an expression inside a branch (e.g. if, for, while) which consists of multiple, logical operators such as AND/OR. The more logical operators in an expression, the more severe the code smell.
[warning] 93-496: ❌ New issue: Bumpy Road Ahead
main has 5 blocks with nested conditional logic. Any nesting of 2 or deeper is considered. Threshold is one single, nested block per function. The Bumpy Road code smell is a function that contains multiple chunks of nested conditional logic. The deeper the nesting and the more bumps, the lower the code health.
[warning] 93-496: ❌ New issue: Deep, Nested Complexity
main has a nested complexity depth of 4, threshold = 4. This function contains deeply nested logic such as if statements and/or loops. The deeper the nesting, the lower the code health.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
tools/authoring/authoring.cc (1)
93-495: 🛠️ Refactor suggestionRefactor the
mainfunction into smaller, focused functions.The
mainfunction has a cyclomatic complexity of 53 (threshold is 9) with deeply nested conditional logic, making it difficult to maintain and test.Consider decomposing
main()into dedicated functions with clearly defined responsibilities:+// Parse and validate command-line arguments +bool parseCommandLineArgs(CommandLine::args& args, char* programName) { + const auto output = args.get<std::string>("o"); + const auto inputs = args.positional(); + const bool asksForHelp = !!args.get<bool>("h"); + const bool hasOutput = output.has_value(); + const bool hasExactlyOneInput = inputs.size() == 1; + + if (asksForHelp || !hasExactlyOneInput || !hasOutput) { + // Display help text + return false; + } + return true; +} + +// Load and validate JSON input file +bool loadAndValidateJsonInput(const std::string& input, nlohmann::json& indexData) { + PCSX::IO<PCSX::File> indexFile(new PCSX::PosixFile(input)); + if (indexFile->failed()) { + // Error handling + return false; + } + + // JSON parsing and validation + return true; +} + +// Additional helper functions for other logical sections + int main(int argc, char** argv) { CommandLine::args args(argc, argv); - // ... existing complex code ... + + if (!parseCommandLineArgs(args, argv[0])) { + return -1; + } + + nlohmann::json indexData; + if (!loadAndValidateJsonInput(inputs[0], indexData)) { + return -1; + } + + // Continue with other extracted functions }🧰 Tools
🪛 GitHub Check: CodeScene Cloud Delta Analysis (main)
[warning] 93-495: ❌ New issue: Complex Method
main has a cyclomatic complexity of 53, threshold = 9. 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] 102-102: ❌ New issue: Complex Conditional
main has 1 complex conditionals with 2 branches, threshold = 2. A complex conditional is an expression inside a branch (e.g. if, for, while) which consists of multiple, logical operators such as AND/OR. The more logical operators in an expression, the more severe the code smell.
[warning] 93-495: ❌ New issue: Bumpy Road Ahead
main has 5 blocks with nested conditional logic. Any nesting of 2 or deeper is considered. Threshold is one single, nested block per function. The Bumpy Road code smell is a function that contains multiple chunks of nested conditional logic. The deeper the nesting and the more bumps, the lower the code health.
[warning] 93-495: ❌ New issue: Deep, Nested Complexity
main has a nested complexity depth of 4, threshold = 4. This function contains deeply nested logic such as if statements and/or loops. The deeper the nesting, the lower the code health.
🧹 Nitpick comments (4)
tools/exe2iso/exe2iso.cc (1)
90-237: Consider refactoring themainfunction to reduce complexity.The main function has high cyclomatic complexity with many conditional statements and nested blocks, making it difficult to maintain and test. Consider extracting logical sections into separate helper functions.
int main(int argc, char** argv) { CommandLine::args args(argc, argv); + + if (!parseCommandLineArgs(args, argv[0])) { + return -1; + } + + if (!openInputOutputFiles(args)) { + return -1; + } + + // Rest of the function broken down into smaller functions + // processLicense(), generateISOStructure(), writeExecutable(), etc. }🧰 Tools
🪛 GitHub Check: CodeScene Cloud Delta Analysis (main)
[warning] 98-194: ❌ Getting worse: Complex Method
main already has high cyclomatic complexity, and now it increases in Lines of Code from 131 to 132. 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.tools/authoring/authoring.cc (1)
264-338: Consider using a thread pool for better resource management.The current implementation creates and detaches threads, which could lead to resource management issues. A thread pool would provide better control over thread lifecycle and resource usage.
+class ThreadPool { +public: + ThreadPool(size_t numThreads) { + for (size_t i = 0; i < numThreads; ++i) { + m_threads.emplace_back([this] { + while (true) { + std::function<void()> task; + { + std::unique_lock lock(m_queueMutex); + m_condition.wait(lock, [this] { return m_stop || !m_tasks.empty(); }); + if (m_stop && m_tasks.empty()) return; + task = std::move(m_tasks.front()); + m_tasks.pop(); + } + task(); + } + }); + } + } + + ~ThreadPool() { + { + std::unique_lock lock(m_queueMutex); + m_stop = true; + } + m_condition.notify_all(); + for (auto& thread : m_threads) { + thread.join(); + } + } + + template<class F> + void enqueue(F&& f) { + { + std::unique_lock lock(m_queueMutex); + m_tasks.emplace(std::forward<F>(f)); + } + m_condition.notify_one(); + } + +private: + std::vector<std::thread> m_threads; + std::queue<std::function<void()>> m_tasks; + std::mutex m_queueMutex; + std::condition_variable m_condition; + bool m_stop = false; +};tools/authoring/README.md (2)
5-5: Minor grammatical improvement needed.The sentence structure in this paragraph could be improved.
-This means the archive manager only requires to have the UCL-NRV2E decompressor registered. +This means the archive manager only requires having the UCL-NRV2E decompressor registered.🧰 Tools
🪛 LanguageTool
[grammar] ~5-~5: Did you mean “having”? Or maybe you should add a pronoun? In active voice, ‘require’ + ‘to’ takes an object, usually a pronoun.
Context: ...means the archive manager only requires to have the UCL-NRV2E decompressor registered. ...(ALLOW_TO)
37-37: Minor grammatical improvements needed.A few minor grammatical corrections would improve readability.
-All the file lookups will be relative to base directory, which is the same as the json file if the option `-basedir` isn't specified. The files will be stored into the archive in their order of appearance. +All the file lookups will be relative to the base directory, which is the same as the json file if the option `-basedir` isn't specified. The files will be stored in the archive in their order of appearance.🧰 Tools
🪛 LanguageTool
[uncategorized] ~37-~37: You might be missing the article “the” here.
Context: ...ll the file lookups will be relative to base directory, which is the same as the jso...(AI_EN_LECTOR_MISSING_DETERMINER_THE)
[uncategorized] ~37-~37: The preposition “in” seems more likely in this position than the preposition “into”.
Context: ...n't specified. The files will be stored into the archive in their order of appearanc...(AI_EN_LECTOR_REPLACEMENT_PREPOSITION_INTO_IN)
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (35)
.github/filter-support/Makefile-filtered(1 hunks).gitignore(1 hunks)Makefile(1 hunks)azure-pipelines-cli.yml(2 hunks)azure-pipelines.yml(2 hunks)src/cdrom/common.h(0 hunks)src/cdrom/file.cc(5 hunks)src/cdrom/file.h(3 hunks)src/cdrom/iso9660-builder.h(0 hunks)src/cdrom/iso9660-reader.cc(1 hunks)src/cdrom/iso9660-reader.h(1 hunks)src/core/luaiso.cc(3 hunks)src/core/web-server.cc(3 hunks)src/mips/common/util/util.h(1 hunks)src/support/binstruct.h(1 hunks)src/support/file.cc(2 hunks)src/support/file.h(3 hunks)src/supportpsx/binffi.lua(1 hunks)src/supportpsx/iec-60908b.h(1 hunks)src/supportpsx/iso9660-builder.cc(4 hunks)src/supportpsx/iso9660-builder.h(1 hunks)src/supportpsx/iso9660-lowlevel.h(3 hunks)tools/authoring/README.md(1 hunks)tools/authoring/authoring.cc(1 hunks)tools/exe2iso/exe2iso.cc(2 hunks)tools/modconv/modconv.cc(2 hunks)tools/ps1-packer/ps1-packer.cc(2 hunks)vsprojects/authoring/authoring.vcxproj(1 hunks)vsprojects/authoring/authoring.vcxproj.filters(1 hunks)vsprojects/cdrom/cdrom.vcxproj(0 hunks)vsprojects/cdrom/cdrom.vcxproj.filters(0 hunks)vsprojects/exe2iso/exe2iso.vcxproj(1 hunks)vsprojects/pcsx-redux.sln(3 hunks)vsprojects/supportpsx/supportpsx.vcxproj(2 hunks)vsprojects/supportpsx/supportpsx.vcxproj.filters(2 hunks)
💤 Files with no reviewable changes (4)
- src/cdrom/iso9660-builder.h
- src/cdrom/common.h
- vsprojects/cdrom/cdrom.vcxproj
- vsprojects/cdrom/cdrom.vcxproj.filters
🚧 Files skipped from review as they are similar to previous changes (24)
- src/cdrom/iso9660-reader.h
- .gitignore
- azure-pipelines.yml
- src/supportpsx/iec-60908b.h
- src/cdrom/iso9660-reader.cc
- azure-pipelines-cli.yml
- vsprojects/authoring/authoring.vcxproj.filters
- src/mips/common/util/util.h
- vsprojects/exe2iso/exe2iso.vcxproj
- src/supportpsx/binffi.lua
- src/support/binstruct.h
- vsprojects/supportpsx/supportpsx.vcxproj.filters
- vsprojects/authoring/authoring.vcxproj
- .github/filter-support/Makefile-filtered
- tools/ps1-packer/ps1-packer.cc
- src/cdrom/file.cc
- tools/modconv/modconv.cc
- Makefile
- src/cdrom/file.h
- src/core/web-server.cc
- src/core/luaiso.cc
- src/supportpsx/iso9660-builder.h
- vsprojects/supportpsx/supportpsx.vcxproj
- vsprojects/pcsx-redux.sln
🧰 Additional context used
🪛 GitHub Check: CodeScene Cloud Delta Analysis (main)
tools/authoring/authoring.cc
[warning] 93-495: ❌ New issue: Complex Method
main has a cyclomatic complexity of 53, threshold = 9. 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] 102-102: ❌ New issue: Complex Conditional
main has 1 complex conditionals with 2 branches, threshold = 2. A complex conditional is an expression inside a branch (e.g. if, for, while) which consists of multiple, logical operators such as AND/OR. The more logical operators in an expression, the more severe the code smell.
[warning] 93-495: ❌ New issue: Bumpy Road Ahead
main has 5 blocks with nested conditional logic. Any nesting of 2 or deeper is considered. Threshold is one single, nested block per function. The Bumpy Road code smell is a function that contains multiple chunks of nested conditional logic. The deeper the nesting and the more bumps, the lower the code health.
[warning] 93-495: ❌ New issue: Deep, Nested Complexity
main has a nested complexity depth of 4, threshold = 4. This function contains deeply nested logic such as if statements and/or loops. The deeper the nesting, the lower the code health.
tools/exe2iso/exe2iso.cc
[warning] 98-194: ❌ Getting worse: Complex Method
main already has high cyclomatic complexity, and now it increases in Lines of Code from 131 to 132. 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] 98-194: ❌ Getting worse: Complex Method
main already has high cyclomatic complexity, and now it increases in Lines of Code from 131 to 132. 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.
🪛 LanguageTool
tools/authoring/README.md
[grammar] ~5-~5: Did you mean “having”? Or maybe you should add a pronoun? In active voice, ‘require’ + ‘to’ takes an object, usually a pronoun.
Context: ...means the archive manager only requires to have the UCL-NRV2E decompressor registered. ...
(ALLOW_TO)
[uncategorized] ~37-~37: You might be missing the article “the” here.
Context: ...ll the file lookups will be relative to base directory, which is the same as the jso...
(AI_EN_LECTOR_MISSING_DETERMINER_THE)
[uncategorized] ~37-~37: The preposition “in” seems more likely in this position than the preposition “into”.
Context: ...n't specified. The files will be stored into the archive in their order of appearanc...
(AI_EN_LECTOR_REPLACEMENT_PREPOSITION_INTO_IN)
[uncategorized] ~41-~41: Loose punctuation mark.
Context: ...ollowing command line arguments: - -o: The output file name for the CD image. ...
(UNLIKELY_OPENING_PUNCTUATION)
[uncategorized] ~42-~42: Loose punctuation mark.
Context: ...or the CD image. Mandatory. - -license: The license file to be used. Optional. ...
(UNLIKELY_OPENING_PUNCTUATION)
[uncategorized] ~43-~43: Loose punctuation mark.
Context: ...ty license on the CD image. - -basedir: The base directory for the files. Optio...
(UNLIKELY_OPENING_PUNCTUATION)
[uncategorized] ~44-~44: Loose punctuation mark.
Context: ...e the json file is located. - -threads: The number of threads to use for compre...
(UNLIKELY_OPENING_PUNCTUATION)
⏰ Context from checks skipped due to timeout of 90000ms (5)
- GitHub Check: Codacy Static Code Analysis
- GitHub Check: aur-build
- GitHub Check: toolchain
- GitHub Check: asan
- GitHub Check: cross-arm64
🔇 Additional comments (20)
src/supportpsx/iso9660-lowlevel.h (2)
1-25: License change from GPL to MITThe license has been changed from GNU GPL to MIT License. This is a significant change that makes the code more permissive, allowing for greater flexibility in how the software can be used, distributed, and modified. This aligns well with the PR objective of adding CD authoring tools, as it removes restrictions that might have limited the tool's integration with other software.
34-34: Modernized namespace declarationThe namespace declaration has been updated from nested namespaces (
namespace PCSX { namespace ISO9660LowLevel {) to the C++17 inline namespace syntax (namespace PCSX::ISO9660LowLevel {). This is a good modernization that simplifies the code while maintaining the same functionality.Also applies to: 136-136
src/support/file.h (3)
344-344: Good: Added Borrow enum for supporting memory borrowing.The
Borrowenum provides a clear type-safe way to distinguish the borrowing behavior in the constructor. This approach makes the API more explicit and type-safe.
356-359: LGTM: Well-documented new constructor for memory borrowing.The added constructor and documentation clearly explain that the BufferFile can borrow memory without taking ownership, and that it won't expand the buffer if writes exceed the size. This is a useful addition for cases where memory is managed externally but needs to be accessed through the BufferFile interface.
377-377: Approved: Enhanced borrow method with offset parameter.Adding the optional offset parameter to the borrow method provides more flexibility, allowing clients to borrow slices from specific positions within the buffer. The default value of 0 maintains backward compatibility with existing code.
src/support/file.cc (3)
55-59: LGTM: Correctly implemented BufferFile constructor with Borrow semantics.The implementation correctly sets up a BufferFile that borrows memory: it initializes the data pointer, sets the size, and marks the buffer as not owned (
m_owned = false). This implementation aligns with the documented behavior in the header file.
155-155: Good: Improved dup() method to consider ownership.The modification to check for both ownership and writability when determining how to duplicate the buffer is appropriate. This ensures that borrowed buffers are handled correctly during duplication.
162-165: LGTM: Correctly implemented borrow method with offset support.The implementation correctly handles the offset by adding it to the data pointer and adjusting the size accordingly. This ensures that the returned slice represents the specified portion of the buffer.
src/supportpsx/iso9660-builder.cc (4)
1-25: LGTM: Updated license to MIT.Changed the license header from GPL to MIT, maintaining proper copyright attribution.
42-43: Consistent namespace updates for SectorMode enum.All references to
SectorModehave been updated to use the fully qualifiedIEC60908b::SectorModename. This change maintains consistency with the code refactoring where the enum was moved to theIEC60908bnamespace.Also applies to: 48-50, 57-58
61-62: LGTM: Updated method signature for writeSectorAt.The method signature has been updated to use
IEC60908b::SectorModeinstead ofSectorMode, which is consistent with the namespace changes throughout the codebase.
69-70: LGTM: Updated all case statements to use fully qualified enum.All case statements in the switch block now use the fully qualified
IEC60908b::SectorModeenum values, maintaining consistency with the namespace changes.Also applies to: 72-73, 81-82, 95-96
tools/exe2iso/exe2iso.cc (2)
98-99: Improved variable immutability withconst.Changing from
autotoconst autoforoutputandinputsenforces immutability, which is good practice since these variables aren't modified after initialization.🧰 Tools
🪛 GitHub Check: CodeScene Cloud Delta Analysis (main)
[warning] 98-194: ❌ Getting worse: Complex Method
main already has high cyclomatic complexity, and now it increases in Lines of Code from 131 to 132. 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.
194-194: Fixed potential header initialization issue.Adding
makeHeaderOnce(sector)ensures the sector header is properly initialized even when no license is written. This prevents potential undefined behavior when writing sectors.🧰 Tools
🪛 GitHub Check: CodeScene Cloud Delta Analysis (main)
[warning] 98-194: ❌ Getting worse: Complex Method
main already has high cyclomatic complexity, and now it increases in Lines of Code from 131 to 132. 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.tools/authoring/authoring.cc (5)
46-56: Well-implemented template function for writing values with endianness handling.This utility function handles both little and big endian byte ordering properly. The use of
constexprconditional evaluation and byte-by-byte writing ensures compatibility across platforms.
60-89: Well-structured bit field implementation for index entries.The
IndexEntryunion effectively uses bit fields to maximize storage efficiency while providing clear accessor methods. The static assertion ensures the size remains as expected (16 bytes).
198-201: Good: Input validation against maximum file count.Checking the file count against the maximum allowed value prevents potential buffer overflows when using the static array.
176-178: Fixed error message to show correct filename.The error message now correctly references the executable path instead of the JSON input filename.
339-344: Good use of lambda functions for code organization.The
putSectorLBAlambda function encapsulates sector LBA writing logic, making the code more readable.tools/authoring/README.md (1)
1-44: Well-organized documentation for the CD Authoring tool.The README provides clear and comprehensive information about the tool's purpose, input format, command-line arguments, and technical details about the archive format and compression.
🧰 Tools
🪛 LanguageTool
[grammar] ~5-~5: Did you mean “having”? Or maybe you should add a pronoun? In active voice, ‘require’ + ‘to’ takes an object, usually a pronoun.
Context: ...means the archive manager only requires to have the UCL-NRV2E decompressor registered. ...(ALLOW_TO)
[uncategorized] ~37-~37: You might be missing the article “the” here.
Context: ...ll the file lookups will be relative to base directory, which is the same as the jso...(AI_EN_LECTOR_MISSING_DETERMINER_THE)
[uncategorized] ~37-~37: The preposition “in” seems more likely in this position than the preposition “into”.
Context: ...n't specified. The files will be stored into the archive in their order of appearanc...(AI_EN_LECTOR_REPLACEMENT_PREPOSITION_INTO_IN)
[uncategorized] ~41-~41: Loose punctuation mark.
Context: ...ollowing command line arguments: --o: The output file name for the CD image. ...(UNLIKELY_OPENING_PUNCTUATION)
[uncategorized] ~42-~42: Loose punctuation mark.
Context: ...or the CD image. Mandatory. --license: The license file to be used. Optional. ...(UNLIKELY_OPENING_PUNCTUATION)
[uncategorized] ~43-~43: Loose punctuation mark.
Context: ...ty license on the CD image. --basedir: The base directory for the files. Optio...(UNLIKELY_OPENING_PUNCTUATION)
[uncategorized] ~44-~44: Loose punctuation mark.
Context: ...e the json file is located. --threads: The number of threads to use for compre...(UNLIKELY_OPENING_PUNCTUATION)
There was a problem hiding this comment.
Actionable comments posted: 0
🔭 Outside diff range comments (1)
tools/exe2iso/exe2iso.cc (1)
93-200: 💡 Verification agent🧩 Analysis chain
Consider refactoring the main function to reduce complexity.
The
mainfunction has high cyclomatic complexity as flagged by static analysis. Consider extracting logical sections into helper functions to improve readability and maintainability.
🏁 Script executed:
#!/bin/bash # Count conditional statements in main function as an indicator of complexity echo "Main function complexity indicators in exe2iso.cc:" echo "Number of if statements: $(grep -c "if\s*(" tools/exe2iso/exe2iso.cc)" echo "Number of for loops: $(grep -c "for\s*(" tools/exe2iso/exe2iso.cc)" echo "Number of while loops: $(grep -c "while\s*(" tools/exe2iso/exe2iso.cc)"Length of output: 428
Action Required: Reduce Main Function Complexity
The static analysis shows that the main function in
tools/exe2iso/exe2iso.ccis fairly complex (14ifstatements and 8forloops). To improve readability and ease future maintenance, please consider refactoring this function. Some suggestions include:
Extracting Functionality:
- Create helper functions for file handling (input, license, and data files).
- Separate the ISO header creation and sector writing logic into their own functions.
Improving Error Handling:
- Consolidate repetitive error checks into reusable routines.
These changes will help in managing complexity and making the codebase easier to understand and maintain.
🧰 Tools
🪛 GitHub Check: CodeScene Cloud Delta Analysis (main)
[warning] 98-194: ❌ Getting worse: Complex Method
main already has high cyclomatic complexity, and now it increases in Lines of Code from 131 to 132. 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.
♻️ Duplicate comments (2)
tools/exe2iso/exe2iso.cc (1)
194-194:⚠️ Potential issueFixed potential header initialization issue.
Adding
makeHeaderOnce(sector)ensures the sector header is properly initialized even when no license is written. This prevents potential undefined behavior when writing sectors.🧰 Tools
🪛 GitHub Check: CodeScene Cloud Delta Analysis (main)
[warning] 98-194: ❌ Getting worse: Complex Method
main already has high cyclomatic complexity, and now it increases in Lines of Code from 131 to 132. 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.tools/authoring/authoring.cc (1)
93-500: 🛠️ Refactor suggestionRefactor the main function into smaller, focused functions.
The
mainfunction has a cyclomatic complexity of 55 (threshold is 9) with deeply nested conditional logic, making it difficult to maintain and test. Consider breaking it down into smaller, more focused functions.#!/bin/bash # Count metrics to verify complexity echo "Main function complexity indicators:" # Count conditional statements echo "Number of if statements: $(grep -c "if\s*(" tools/authoring/authoring.cc)" # Count for loops: $(grep -c "for\s*(" tools/authoring/authoring.cc)" # Count while loops: $(grep -c "while\s*(" tools/authoring/authoring.cc)" # Count early returns echo "Number of return statements: $(grep -c "return -1;" tools/authoring/authoring.cc)"Suggested structure to refactor into:
parseCommandLineArgs()- Handle argument parsing and validationloadAndValidateJsonInput()- Load and validate the JSON structuresetupOutputAndLicenseFiles()- Open output file and optional license fileprocessExecutable()- Handle executable loading and processingcompressFiles()- Set up threading and compress fileswriteSectors()- Write all sectors to the output filecreateISOStructure()- Create ISO9660 structureEach would have clear inputs, outputs, and error handling.
🧰 Tools
🪛 GitHub Check: CodeScene Cloud Delta Analysis (main)
[warning] 93-500: ❌ New issue: Complex Method
main has a cyclomatic complexity of 55, threshold = 9. 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] 102-102: ❌ New issue: Complex Conditional
main has 1 complex conditionals with 2 branches, threshold = 2. A complex conditional is an expression inside a branch (e.g. if, for, while) which consists of multiple, logical operators such as AND/OR. The more logical operators in an expression, the more severe the code smell.
[warning] 93-500: ❌ New issue: Bumpy Road Ahead
main has 5 blocks with nested conditional logic. Any nesting of 2 or deeper is considered. Threshold is one single, nested block per function. The Bumpy Road code smell is a function that contains multiple chunks of nested conditional logic. The deeper the nesting and the more bumps, the lower the code health.
[warning] 93-500: ❌ New issue: Deep, Nested Complexity
main has a nested complexity depth of 4, threshold = 4. This function contains deeply nested logic such as if statements and/or loops. The deeper the nesting, the lower the code health.
🧹 Nitpick comments (6)
tools/authoring/authoring.cc (3)
264-343: Consider using a thread pool instead of detached threads.The current implementation creates and detaches threads in a loop, which doesn't provide control over thread lifecycle or proper cleanup. A thread pool would be more efficient and safer.
- for (unsigned i = 0; i < threadCount; i++) { - std::thread t([&]() { - while (1) { - // thread work - } - }); - t.detach(); - } + std::vector<std::thread> threadPool; + for (unsigned i = 0; i < threadCount; i++) { + threadPool.emplace_back([&]() { + while (1) { + // thread work + } + }); + } + + // After processing is complete + for (auto& thread : threadPool) { + thread.join(); + }
245-245: Replace static array with a dynamic container.Using a static array with a predefined maximum size is prone to errors and inflexible. Consider using a vector instead:
- static WorkUnit work[c_maximumSectorCount]; + std::vector<WorkUnit> work(filesCount);This would automatically allocate the right amount of memory and eliminate potential out-of-bounds access concerns.
267-268: Fix infinite loop with proper exit condition.The thread function uses
while(1)which creates an infinite loop, relying on thereturnstatement to exit. This pattern is error-prone. Consider using a more explicit condition:- std::thread t([&]() { - while (1) { + std::thread t([&]() { + while (true) {While functionally equivalent, using
while(true)more clearly indicates your intent for an infinite loop with internal exit conditions.tools/authoring/README.md (3)
5-5: Grammar improvement in requirement description.-This means the archive manager only requires to have the UCL-NRV2E decompressor registered. +This means the archive manager only requires the UCL-NRV2E decompressor to be registered.🧰 Tools
🪛 LanguageTool
[grammar] ~5-~5: Did you mean “having”? Or maybe you should add a pronoun? In active voice, ‘require’ + ‘to’ takes an object, usually a pronoun.
Context: ...means the archive manager only requires to have the UCL-NRV2E decompressor registered. ...(ALLOW_TO)
37-37: Minor grammar improvements.-All the file lookups will be relative to base directory, which is the same as the json file if the option `-basedir` isn't specified. The files will be stored into the archive in their order of appearance. +All the file lookups will be relative to the base directory, which is the same as the json file if the option `-basedir` isn't specified. The files will be stored in the archive in their order of appearance.🧰 Tools
🪛 LanguageTool
[uncategorized] ~37-~37: You might be missing the article “the” here.
Context: ...ll the file lookups will be relative to base directory, which is the same as the jso...(AI_EN_LECTOR_MISSING_DETERMINER_THE)
[uncategorized] ~37-~37: The preposition “in” seems more likely in this position than the preposition “into”.
Context: ...n't specified. The files will be stored into the archive in their order of appearanc...(AI_EN_LECTOR_REPLACEMENT_PREPOSITION_INTO_IN)
41-44: Improved command-line arguments formatting.The list formatting for command-line arguments could be improved for better readability.
-- `-o`: The output file name for the CD image. Mandatory. -- `-license`: The license file to be used. Optional. Point to an existing valid PlayStation 1 disc, or an official license file. The tool will use the license file to generate the CD image. If not provided, the tool will generate an empty license on the CD image. -- `-basedir`: The base directory for the files. Optional. Default is the directory where the json file is located. -- `-threads`: The number of threads to use for compression. Optional. Default is the number of available CPU cores. +- **`-o`**: The output file name for the CD image. **Mandatory**. +- **`-license`**: The license file to be used. *Optional*. Point to an existing valid PlayStation 1 disc, or an official license file. The tool will use the license file to generate the CD image. If not provided, the tool will generate an empty license on the CD image. +- **`-basedir`**: The base directory for the files. *Optional*. Default is the directory where the json file is located. +- **`-threads`**: The number of threads to use for compression. *Optional*. Default is the number of available CPU cores.🧰 Tools
🪛 LanguageTool
[uncategorized] ~41-~41: Loose punctuation mark.
Context: ...ollowing command line arguments: --o: The output file name for the CD image. ...(UNLIKELY_OPENING_PUNCTUATION)
[uncategorized] ~42-~42: Loose punctuation mark.
Context: ...or the CD image. Mandatory. --license: The license file to be used. Optional. ...(UNLIKELY_OPENING_PUNCTUATION)
[uncategorized] ~43-~43: Loose punctuation mark.
Context: ...ty license on the CD image. --basedir: The base directory for the files. Optio...(UNLIKELY_OPENING_PUNCTUATION)
[uncategorized] ~44-~44: Loose punctuation mark.
Context: ...e the json file is located. --threads: The number of threads to use for compre...(UNLIKELY_OPENING_PUNCTUATION)
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (35)
.github/filter-support/Makefile-filtered(1 hunks).gitignore(1 hunks)Makefile(1 hunks)azure-pipelines-cli.yml(2 hunks)azure-pipelines.yml(2 hunks)src/cdrom/common.h(0 hunks)src/cdrom/file.cc(5 hunks)src/cdrom/file.h(3 hunks)src/cdrom/iso9660-builder.h(0 hunks)src/cdrom/iso9660-reader.cc(1 hunks)src/cdrom/iso9660-reader.h(1 hunks)src/core/luaiso.cc(3 hunks)src/core/web-server.cc(3 hunks)src/mips/common/util/util.h(1 hunks)src/support/binstruct.h(1 hunks)src/support/file.cc(2 hunks)src/support/file.h(3 hunks)src/supportpsx/binffi.lua(1 hunks)src/supportpsx/iec-60908b.h(1 hunks)src/supportpsx/iso9660-builder.cc(4 hunks)src/supportpsx/iso9660-builder.h(1 hunks)src/supportpsx/iso9660-lowlevel.h(3 hunks)tools/authoring/README.md(1 hunks)tools/authoring/authoring.cc(1 hunks)tools/exe2iso/exe2iso.cc(2 hunks)tools/modconv/modconv.cc(2 hunks)tools/ps1-packer/ps1-packer.cc(2 hunks)vsprojects/authoring/authoring.vcxproj(1 hunks)vsprojects/authoring/authoring.vcxproj.filters(1 hunks)vsprojects/cdrom/cdrom.vcxproj(0 hunks)vsprojects/cdrom/cdrom.vcxproj.filters(0 hunks)vsprojects/exe2iso/exe2iso.vcxproj(1 hunks)vsprojects/pcsx-redux.sln(3 hunks)vsprojects/supportpsx/supportpsx.vcxproj(2 hunks)vsprojects/supportpsx/supportpsx.vcxproj.filters(2 hunks)
💤 Files with no reviewable changes (4)
- src/cdrom/common.h
- src/cdrom/iso9660-builder.h
- vsprojects/cdrom/cdrom.vcxproj.filters
- vsprojects/cdrom/cdrom.vcxproj
🚧 Files skipped from review as they are similar to previous changes (24)
- .gitignore
- src/supportpsx/binffi.lua
- src/cdrom/iso9660-reader.h
- src/mips/common/util/util.h
- src/supportpsx/iec-60908b.h
- azure-pipelines-cli.yml
- azure-pipelines.yml
- .github/filter-support/Makefile-filtered
- src/cdrom/iso9660-reader.cc
- vsprojects/authoring/authoring.vcxproj
- vsprojects/authoring/authoring.vcxproj.filters
- vsprojects/exe2iso/exe2iso.vcxproj
- tools/ps1-packer/ps1-packer.cc
- src/core/luaiso.cc
- vsprojects/supportpsx/supportpsx.vcxproj.filters
- Makefile
- tools/modconv/modconv.cc
- src/cdrom/file.cc
- src/cdrom/file.h
- src/supportpsx/iso9660-lowlevel.h
- src/support/binstruct.h
- vsprojects/supportpsx/supportpsx.vcxproj
- src/supportpsx/iso9660-builder.h
- vsprojects/pcsx-redux.sln
🧰 Additional context used
🪛 GitHub Check: CodeScene Cloud Delta Analysis (main)
tools/authoring/authoring.cc
[warning] 93-500: ❌ New issue: Complex Method
main has a cyclomatic complexity of 55, threshold = 9. 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] 102-102: ❌ New issue: Complex Conditional
main has 1 complex conditionals with 2 branches, threshold = 2. A complex conditional is an expression inside a branch (e.g. if, for, while) which consists of multiple, logical operators such as AND/OR. The more logical operators in an expression, the more severe the code smell.
[warning] 93-500: ❌ New issue: Bumpy Road Ahead
main has 5 blocks with nested conditional logic. Any nesting of 2 or deeper is considered. Threshold is one single, nested block per function. The Bumpy Road code smell is a function that contains multiple chunks of nested conditional logic. The deeper the nesting and the more bumps, the lower the code health.
[warning] 93-500: ❌ New issue: Deep, Nested Complexity
main has a nested complexity depth of 4, threshold = 4. This function contains deeply nested logic such as if statements and/or loops. The deeper the nesting, the lower the code health.
tools/exe2iso/exe2iso.cc
[warning] 98-194: ❌ Getting worse: Complex Method
main already has high cyclomatic complexity, and now it increases in Lines of Code from 131 to 132. 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] 98-194: ❌ Getting worse: Complex Method
main already has high cyclomatic complexity, and now it increases in Lines of Code from 131 to 132. 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.
🪛 LanguageTool
tools/authoring/README.md
[grammar] ~5-~5: Did you mean “having”? Or maybe you should add a pronoun? In active voice, ‘require’ + ‘to’ takes an object, usually a pronoun.
Context: ...means the archive manager only requires to have the UCL-NRV2E decompressor registered. ...
(ALLOW_TO)
[uncategorized] ~37-~37: You might be missing the article “the” here.
Context: ...ll the file lookups will be relative to base directory, which is the same as the jso...
(AI_EN_LECTOR_MISSING_DETERMINER_THE)
[uncategorized] ~37-~37: The preposition “in” seems more likely in this position than the preposition “into”.
Context: ...n't specified. The files will be stored into the archive in their order of appearanc...
(AI_EN_LECTOR_REPLACEMENT_PREPOSITION_INTO_IN)
[uncategorized] ~41-~41: Loose punctuation mark.
Context: ...ollowing command line arguments: - -o: The output file name for the CD image. ...
(UNLIKELY_OPENING_PUNCTUATION)
[uncategorized] ~42-~42: Loose punctuation mark.
Context: ...or the CD image. Mandatory. - -license: The license file to be used. Optional. ...
(UNLIKELY_OPENING_PUNCTUATION)
[uncategorized] ~43-~43: Loose punctuation mark.
Context: ...ty license on the CD image. - -basedir: The base directory for the files. Optio...
(UNLIKELY_OPENING_PUNCTUATION)
[uncategorized] ~44-~44: Loose punctuation mark.
Context: ...e the json file is located. - -threads: The number of threads to use for compre...
(UNLIKELY_OPENING_PUNCTUATION)
⏰ Context from checks skipped due to timeout of 90000ms (8)
- GitHub Check: pcsx-redux (x86_64-linux)
- GitHub Check: Codacy Static Code Analysis
- GitHub Check: toolchain
- GitHub Check: aur-build
- GitHub Check: cross-arm64
- GitHub Check: build
- GitHub Check: coverage
- GitHub Check: asan
🔇 Additional comments (16)
src/support/file.h (3)
344-344: Addition of Borrow enum enhances memory handling options.The new
Borrowenum provides a clear way to indicate that aBufferFileshould borrow memory rather than copying or taking ownership of it. This is a good pattern for memory efficiency.
356-359: Well-documented constraints for the new borrowed memory constructor.The comment clearly explains the behavior and limitations of the borrowed memory approach - specifically that the buffer will not expand if writes exceed its current size. This is important information for users of this API.
377-377: Enhanced flexibility with optional offset parameter.Adding the default offset parameter to the
borrow()method provides more flexibility when creating slices, allowing clients to borrow portions starting at arbitrary offsets without changing existing code that uses this method.src/support/file.cc (3)
55-59: Good implementation of the Borrow constructor.The implementation properly sets up a
BufferFilethat references external memory without taking ownership, consistent with the documentation in the header file. Them_owned = falseflag ensures this memory won't be freed when theBufferFileis destroyed.
155-155: Improved logic in dup() method.The updated condition improves the logic for determining how to duplicate a buffer, now checking both ownership and writability status. This change ensures proper duplication behavior based on the buffer's characteristics.
162-164: Updated borrow() method uses new offset parameter.The implementation properly uses the offset parameter to create a slice starting at the specified position in the buffer. The size calculation correctly adjusts for the offset to maintain proper bounds.
src/supportpsx/iso9660-builder.cc (2)
1-25: License updated from GPL to MIT.The license has been changed from GNU General Public License to MIT License, which is more permissive. This appears to be part of a broader effort to standardize licensing across the codebase.
42-43: Consistent namespace update for SectorMode.All references to
SectorModehave been updated to use the fully qualified nameIEC60908b::SectorMode, improving code organization and preventing namespace collisions. This change is part of a broader refactoring effort visible across multiple files.Also applies to: 48-50, 57-58, 61-62, 69-70, 72-73, 81-82, 95-96
src/core/web-server.cc (2)
46-46: Updated include path reflects file reorganization.The include path has been updated from
cdrom/iso9660-builder.htosupportpsx/iso9660-builder.h, reflecting a reorganization of files to better represent their purpose and dependencies.
466-466: Consistent update to IEC60908b namespace.The type of the
modevariable and its associated enum casts have been updated to usePCSX::IEC60908b::SectorMode, maintaining consistency with the namespace changes seen in other files. This is a good example of thoroughness in refactoring.Also applies to: 476-476
tools/exe2iso/exe2iso.cc (1)
98-99: Improved variable safety with const qualifiers.Adding
constto these variables ensures they can't be accidentally modified after initialization, enhancing code safety and making intentions clearer to readers.🧰 Tools
🪛 GitHub Check: CodeScene Cloud Delta Analysis (main)
[warning] 98-194: ❌ Getting worse: Complex Method
main already has high cyclomatic complexity, and now it increases in Lines of Code from 131 to 132. 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.tools/authoring/authoring.cc (3)
46-56: Well-implemented endianness-aware buffer writing function.The
writeToBuffertemplate function correctly handles endianness conversion when needed, ensuring proper cross-platform compatibility.
60-89: Well-designed IndexEntry union with BitField.The
IndexEntryunion uses bit fields efficiently to maximize storage in a 16-byte structure, with clear accessor methods for all fields.
194-201: Good validation against maximum sector count.Validating the file count against the maximum allowed number of sectors (line 198-201) prevents potential buffer overflows in the static work array.
tools/authoring/README.md (2)
1-8: Clear introduction and overview of the CD Authoring tool.The README provides a good overview of the tool's purpose and functionality, with appropriate links to related files for more information.
🧰 Tools
🪛 LanguageTool
[grammar] ~5-~5: Did you mean “having”? Or maybe you should add a pronoun? In active voice, ‘require’ + ‘to’ takes an object, usually a pronoun.
Context: ...means the archive manager only requires to have the UCL-NRV2E decompressor registered. ...(ALLOW_TO)
9-33: Well-structured JSON example.The JSON example clearly illustrates the expected format with all available options, making it easy for users to understand how to configure the tool.
No description provided.