refactor(MachOSwiftSection): extract _sectionOffsetAndSize helper#81
refactor(MachOSwiftSection): extract _sectionOffsetAndSize helper#81
Conversation
Dedupe repeated section offset/size computation across _readDescriptors, _readRelativeDescriptors, _readTypeMetadataRecords, and _readProtocolRecords in both MachOFile.Swift and MachOImage.Swift.
There was a problem hiding this comment.
Pull request overview
This PR refactors MachOSwiftSection’s Mach-O Swift section readers by extracting a shared _sectionOffsetAndSize(of:in:) helper for both MachOImage (slide-aware) and MachOFile (dyld-cache-aware), reducing duplicated offset/size calculations while keeping behavior the same.
Changes:
- Add
_sectionOffsetAndSize(of:in:)helper inMachOImage.SwiftandMachOFile.Swift. - Update
_readDescriptors,_readRelativeDescriptors,_readTypeMetadataRecords, and_readProtocolRecordsto use the helper.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| Sources/MachOSwiftSection/MachOImage+Swift.swift | Deduplicates section offset/size computation for in-memory images and updates section-reading helpers to reuse it. |
| Sources/MachOSwiftSection/MachOFile+Swift.swift | Deduplicates section offset/size computation for file-backed Mach-Os (including cache-aware offsets) and updates section-reading helpers to reuse it. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
![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 refactors MachOFile.Swift and MachOImage.Swift by extracting the logic for calculating section offsets and sizes into a new private helper method, _sectionOffsetAndSize. This change reduces code duplication within each extension. The review feedback suggests further improvements, including removing redundant method parameters, using the more idiomatic distance(to:) method for pointer calculations, and exploring a shared protocol to unify the now-identical implementations across both files.
|
|
||
| extension MachOFile.Swift { | ||
| private func _readDescriptors<Descriptor: TopLevelDescriptor>(from swiftMachOSection: MachOSwiftSectionName, in machO: MachOFile) throws -> [Descriptor] { | ||
| private func _sectionOffsetAndSize(of swiftMachOSection: MachOSwiftSectionName, in machO: MachOFile) throws -> (offset: Int, size: Int) { |
There was a problem hiding this comment.
The machO parameter is redundant in this and other private helper methods because it is already available as a property of the Swift struct (self.machO). Removing it would simplify the method signatures and call sites across both MachOFile.Swift and MachOImage.Swift extensions.
| private func _sectionOffsetAndSize(of swiftMachOSection: MachOSwiftSectionName, in machO: MachOFile) throws -> (offset: Int, size: Int) { | |
| private func _sectionOffsetAndSize(of swiftMachOSection: MachOSwiftSectionName) throws -> (offset: Int, size: Int) { |
| return (offset, section.size) | ||
| } | ||
|
|
||
| private func _readDescriptors<Descriptor: TopLevelDescriptor>(from swiftMachOSection: MachOSwiftSectionName, in machO: MachOFile) throws -> [Descriptor] { |
There was a problem hiding this comment.
The implementation of _readDescriptors (and the other _read... methods below) is now identical to the implementation in MachOImage+Swift.swift. To improve maintainability and avoid future divergence, consider extracting these into a shared protocol extension. You could define an internal protocol that provides the _sectionOffsetAndSize and reading methods, and have both MachOFile and MachOImage (or their Swift wrappers) conform to it.
| var descriptors: [Descriptor] = [] | ||
| let vmaddrSlide = try required(machO.vmaddrSlide) | ||
| let start = try required(UnsafeRawPointer(bitPattern: section.address + vmaddrSlide)) | ||
| let offset = start.bitPattern.int - machO.ptr.bitPattern.int |
There was a problem hiding this comment.
Use distance(to:) for a more idiomatic and readable way to calculate the offset between two pointers in Swift. This avoids manual bit pattern manipulation and integer conversions.
| let offset = start.bitPattern.int - machO.ptr.bitPattern.int | |
| let offset = machO.ptr.distance(to: start) |
- Drop redundant machO parameter from private _read* and _sectionOffsetAndSize helpers since self.machO is already available. - Use UnsafeRawPointer.distance(to:) in place of manual bitPattern subtraction in MachOImage variant. - Move `extension RelativeDirectPointer: LayoutProtocol` into a dedicated file under MachOPointers, alongside the pointer definition.
Summary
_sectionOffsetAndSize(of:in:)in bothMachOFile.SwiftandMachOImage.Swiftto compute a section's offset (cache-aware forMachOFile, slide-aware forMachOImage) and size once._readDescriptors,_readRelativeDescriptors,_readTypeMetadataRecords, and_readProtocolRecordsto call the new helper instead of repeating the offset/size computation inline.Test plan
swift buildswift test --filter MachOSwiftSectionTestsswift test --filter SwiftDumpTests