Suggestion: Wrapping malloc/free-able resources in "RAII" classes on the Swift side

[DivineDominion]

In @ricobeck's presentation today we looked at this part:

PlaydateKit/Sources/PlaydateKit/Core/Sound.swift

Lines 227 to 237 in 03ee2e1

	@discardableResult public func setSample(path: UnsafePointer<CChar>) -> Bool {
	if let samplePointer {
	sample.freeSample(samplePointer)
	}
	samplePointer = sample.load(path)
	guard samplePointer != nil else { return false }
	sampleplayer.setSample(playerPointer, samplePointer)
	return true
	}

Suggestion

What do you think about this:

We wrap a manually memory-managed object like "Sample" in a RAII (Resource Acquisition Is Initialization) class. (I know it's a C++ technique https://en.cppreference.com/w/cpp/language/raii but works with C as well to hide pointer mangement)

Before

 @discardableResult public func setSample(path: UnsafePointer<CChar>) -> Bool { 
     // Every change to self.sample needs to care about memory
     if let samplePointer { 
         sample.freeSample(samplePointer) 
     } 
  
     // Replacing can fail (but the other pointer is already gone)
     samplePointer = sample.load(path) 
     guard samplePointer != nil else { return false } 
  
     // Still using raw pointers after the load, also for playerPointer
     sampleplayer.setSample(playerPointer, samplePointer) 
     return true 
 } 

After

 @discardableResult public func setSample(path: UnsafePointer<CChar>) -> Bool { 
     guard let newSample = Sample(loadFromPath: path) else { return false }
     self.sample = newSample  
     newSample.withUnsafePointer { samplePointer in
        sampleplayer.setSample(playerPointer, samplePointer) 
     }
     return true 
 } 

Approach

Pseudoswift sketch:

final class Sample {
    private let pointer: OpaquePointer

    init?(loadFromPath path: UnsafePointer<CChar>) {
        let pointer = sample.load(path)
        guard pointer != nil else { return nil }
        self.pointer = pointer
    }

    deinit {
        sample.freeSample(pointer)
    }

    func withUnsafePointer(_ body: (OpaquePointer) -> Void) {
        body(pointer)
    }
}

• Upside: if you have a Sample, your pointer is still alive, and we can use ARC to manage Sample's lifetime. So no calls to 'free' outside of where the opaque pointers are privately known.
• Downside: It's another heap allocation.

[finnvoor]

Ah nice, is there any recording of this presentation?

Since this is all implementation details internal to PlaydateKit I don't really mind how we deal with unsafe pointers, but wrapping them like this is probably a bit more future proof in case more sample methods are added.

[DivineDominion]

It was a live thing at a CocoaHeads meetup without recording as far as I know. I'll ask @ricobeck to double check :)

Since this is all implementation details internal to PlaydateKit I don't really mind how we deal with unsafe pointers, but wrapping them like this is probably a bit more future proof in case more sample methods are added.

This is totally targeted at keeping PlaydateKit bug-free and maintainable with potentially multiple people not well-versed in C working on the code base. So that'd be the tradeoff.

I also forgot to mention that the existing code base already does this at important points (including the cited file!). My proposal would be to consistently wrap everything with a name and an opaque pointer under the hood this way to make them all a bit more same-y. The benefit of that would then of course be predictability of behavior, and maybe helping with project onboarding to focus on the Swiftiness instead of worrying about pointers.

That's all pretty vague of course and a short in the dark :)

This doesn't have to happen overnight (or at all), but I'd love to start this and give it a shot if nobody complains 👀

[ricobeck]

(As for my Cocoaheads talk: unfortunately there is no recording. It was our first attempt at streaming. It would also have been in German. 😅)

[finnvoor]

@DivineDominion that all sounds good to me, happy to accept any PRs for this

[DivineDominion]

I believe I'll have some spare time eventually in the next months, so I can finally have a look :)