Conversation
Introduce a new macro in utils.rs to encapsulate sorting and iterating over a directory's children by name. Replace duplicate sorting logic in iso_writer.rs functions (write_directories and copy_files) with this macro, improving code reusability and reducing duplication.
- Extract directory path creation into reusable ensure_directory_path utility - Simplify add_file method by removing manual path traversal logic - Introduce prepare_boot_entries method for bios and uefi boot catalog prep - Add write_hybrid_structures method to centralize MBR and GPT writing - Improves code maintainability and reduces duplication in boot setup
Introduce PathContext struct in builder_utils.rs to cache path components and validation, avoiding repeated parsing in path operations. Extract helper function in builder.rs for conditional boot entry creation, reducing code duplication and improving readability in ISO boot configuration. This refactoring enhances performance for frequent path accesses and simplifies logic for BIOS and UEFI boot entries. ``` refactor: optimize path parsing and refactor boot entry addition Introduce PathContext struct in builder_utils.rs to cache path components and validation, avoiding repeated parsing in path operations. Extract helper function in builder.rs for conditional boot entry creation, reducing code duplication and improving readability in ISO boot configuration. This refactoring enhances performance for frequent path accesses and simplifies logic for BIOS and UEFI boot entries.
…h BootType enum Add BootType enum (Bios, Uefi, UefiEsp) with platform_id and description methods. Replace create_boot_entry with create_boot_entry_generic to handle all boot types, refactor create_bios_boot_entry, create_uefi_boot_entry, and create_uefi_esp_boot_entry to use the generic function, eliminating duplication and improving maintainability.
Remove generic helper function `add_boot_entry_if_some` in `builder.rs` and inline conditional logic using `if let` patterns for BIOS and UEFI boot entries. Refactor `create_boot_entry_generic` in `builder_utils.rs` to unify `BootCatalogEntry` construction, embedding the boot image LBA calculation via match within the struct initialization to eliminate code duplication. This improves readability and maintainability without altering functionality.
Summary of ChangesHello @p14c31355, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request undertakes a significant refactoring effort to enhance the structure, readability, and maintainability of the ISO image creation logic. It introduces several helper functions and a new macro to generalize common operations, such as handling boot catalog entries, managing directory paths, and writing dual-endian data, resulting in a more modular and robust codebase. Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a significant and valuable refactoring of the ISO building logic. By introducing helper functions, generic methods, and macros, the code is now more modular, readable, and has less duplication. The changes are well-structured and improve the overall design. I have a few minor suggestions to further enhance maintainability and performance by restoring some explanatory comments, using constants instead of magic numbers, and avoiding a redundant sorting operation.
| total_lbas: u64, | ||
| esp_size_sectors: Option<u32>, | ||
| ) -> io::Result<()> { | ||
| // GPT structures require at least 69 LBAs |
There was a problem hiding this comment.
The comment here is less descriptive than the one it replaced. The previous comment explained why 69 LBAs are required, which is valuable information for future maintenance. It would be good to restore that detail.
For example:
// GPT structures require at least 69 LBAs (1 MBR + 1 GPT Header + 32 Partition Entries + 1 Backup GPT Header + 32 Backup Partition Entries + 2 for safety)
|
|
||
| let mut sorted_children: Vec<_> = dir.children.iter().collect(); | ||
| sorted_children.sort_by_key(|(name, _)| *name); |
There was a problem hiding this comment.
The logic to sort children was removed here and replaced by two separate calls to the for_sorted_children! macro later in the function (lines 61 and 95). This causes the same list of children to be collected and sorted twice, which is inefficient. It would be more performant to sort the children once into a vector and then iterate over that vector twice, as was done in the original code.
Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
1 participant
Change details
Build / Test Results