Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThis pull request adds support for ISO9660 Supplementary Volume Descriptors with Joliet encoding detection. It introduces new struct definitions, a Joliet escape sequence matcher, UCS-2BE to UTF-8 conversion utilities, and extends the ISO printing logic to detect and display Joliet volume identifiers. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![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 (1)
filesystem/iso9660/iso9660_defs.ixx (1)
172-213: Add a size guard for the packed descriptor layout.Since this struct maps raw on-disk bytes, add a compile-time size check to catch accidental field/layout drift.
♻️ Proposed refactor
struct SupplementaryVolumeDescriptor { @@ uint8_t reserved2[653]; }; + +static_assert(sizeof(SupplementaryVolumeDescriptor) == sizeof(VolumeDescriptor), + "SupplementaryVolumeDescriptor must remain 2048 bytes"); `#pragma` pack(pop)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@filesystem/iso9660/iso9660_defs.ixx` around lines 172 - 213, Add a compile-time size guard right after the SupplementaryVolumeDescriptor definition to ensure the packed on-disk layout hasn't drifted; e.g. add a static_assert(sizeof(SupplementaryVolumeDescriptor) == 2048, "SupplementaryVolumeDescriptor size must match on-disk layout (2048)"); ensure this assertion references the struct name SupplementaryVolumeDescriptor and use the expected on-disk size (2048 bytes) so any accidental field/layout changes fail at compile time.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@filesystem/joliet/joliet_defs.ixx`:
- Around line 25-53: The identifier_to_string function currently UTF-8-encodes
UCS-2BE codepoints and then calls trim(result).c_str(), which is undefined
because trim uses std::isspace on signed char values and also returns a pointer
into a temporary. Fix: keep the signature identifier_to_string(const uint16_t
(&identifier)[N]) and the endian_swap/UTF-8 encoding logic, remove the trim()
call, and instead manually strip only ASCII space characters (byte 0x20) from
the end of result (e.g., while(!result.empty() && result.back() == ' ')
result.pop_back();), then return the result string (not result.c_str()). Ensure
no use of std::isspace on raw chars.
In `@systems/s_joliet.ixx`:
- Around line 34-38: The comparison using std::ranges::find against
svd.escape_sequences is wrong because svd.escape_sequences is a 32-byte char
array while joliet::ESCAPE_SEQUENCES holds 3-char sequence string_views; change
the comparison to build a std::string_view over only the actual 3-byte escape
sequence (e.g. std::string_view(svd.escape_sequences, 3)) and compare that
against joliet::ESCAPE_SEQUENCES (or use std::ranges::find with that
string_view). Keep the existing cast for volume_identifier and leave
iso9660::Browser::findDescriptor, svd.escape_sequences,
joliet::ESCAPE_SEQUENCES, std::ranges::find, and joliet::identifier_to_string
references intact.
---
Nitpick comments:
In `@filesystem/iso9660/iso9660_defs.ixx`:
- Around line 172-213: Add a compile-time size guard right after the
SupplementaryVolumeDescriptor definition to ensure the packed on-disk layout
hasn't drifted; e.g. add a static_assert(sizeof(SupplementaryVolumeDescriptor)
== 2048, "SupplementaryVolumeDescriptor size must match on-disk layout (2048)");
ensure this assertion references the struct name SupplementaryVolumeDescriptor
and use the expected on-disk size (2048 bytes) so any accidental field/layout
changes fail at compile time.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: e1ac712a-4ec6-4666-bb2f-49f58a5ef287
📒 Files selected for processing (6)
CMakeLists.txtfilesystem/iso9660/iso9660_defs.ixxfilesystem/joliet/joliet.ixxfilesystem/joliet/joliet_defs.ixxsystems/s_joliet.ixxsystems/systems.ixx
superg
left a comment
superg
left a comment
There was a problem hiding this comment.
Two things:
- I would like you to move joliet label handling to SystemISO, as it's an extension to ISO9660, I'd rather have that implemented there (please do it under PVD label printout)
- Please use codecvt to convert (with endianness fix first), something like:
std::wstring_convert<std::codecvt_utf8_utf16<char16_t>, char16_t> convert;
std::string utf8_raw = convert.to_bytes(host_u16);
(I know codecvt is kind of deprecated but into standard committee figure this out, it's available)
After that these files should disappear:
filesystem/joliet/joliet.ixx
filesystem/joliet/joliet_defs.ixx
systems/s_joliet.ixx
|
I initially made it into a separate system for better readability, and also in case you want to expand joliet support in the future, like for skeleton generation for example. Regarding the string conversion, that's fine to me, but the compiler will complain about it. |
|
Updated example of generated output: |
2 participants
This PR adds joliet volume identifier logging.
To do this, it parses the iso 9660 supplementary volume descriptor (if available), checks for UCS-2 escape sequences and converts the volume identifier from UCS-2BE to UTF-8.
Example of generated output:
Summary by CodeRabbit