Add hsDarwin.h with helpers for string conversion

[dpogue]

The NSString category in the macOS plClient folder is useful, but there are several other spots where we're wanting to convert between CFString/NSString and ST::string, so it make sense to have helpers available throughout the engine. These helper functions are written so that they can be used from C++ code without needing to bring in Objective-C.

[colincornaby]

My general thoughts on this...

I don't really see a problem - but like @dgelessus mentioned it would be nice to leave the Obj-C category intact. Part of my goals has been to provide a sane frontend for any future Swift code through Obj-C. This new code doesn't include nullability annotations - which is a step back. The existing Obj-C code could be refactored to sit on this new code however.

I think my perspective is also that Obj-C (or Swift) is always going to be implicitly part of writing an iOS or Mac app. hsMessageBox_Mac.mm is a good example of this. Maybe not directly related to this PR - but in general I'll be a little skeptical of code sections where C++ is used to avoid Obj-C for no clear reason. Objective-C++ can also be used to patch Obj-C into C++ classes (again with hsMessageBox_Mac.mm being an example.) CFStringRef should only be used where the integration directly requires it (like in keychain support.)

Also going to note that CoreFoundation is manually memory managed - while Obj-C is automatically memory managed. I'll check through this commit to see if that's properly handled. But needing memory management review is a reason I'd discourage use of CoreFoundation unless appropriate.

[colincornaby]

Reading through - it would be better if the Obj-C interface was left working and returned an autoreleased string. There are a few other possible ways (changing the bridge to bridge_transfer for example) - but leaving the Obj-C interface intact would be the most unambiguous on what the memory rules are.

A __bridge_transfer might still be necessary in the Obj-C wrapper - but it's better to do that changeover at the exact point that a CFString crosses from C to Obj-C. I worry that doing a __bridge_transfer inside of a C function is ambiguous.

[dpogue]

A __bridge_transfer might still be necessary in the Obj-C wrapper - but it's better to do that changeover at the exact point that a CFString crosses from C to Obj-C. I worry that doing a __bridge_transfer inside of a C function is ambiguous.

It looks like CFBridgingRetain is the correct way to do this from a C function

[colincornaby]

I'm still not super excited about the removal of the Obj-C category - it could still be a front end for the new code. And it's a nice affordance to have when working in Obj-C. But I think the changes are an improvement for classes like the keychain handling. There would also be less churn in the Obj-C code if a category was maintained.

But I think in principal these are good changes - and the latest edits fix the memory issues.

[dpogue]

Like I said, I don't mind putting the ObjC category back, but that still raises issues around using it in spots outside of plClient (such as the caption/message of hsMessageBox).

I've (minimally) tested that this code works in 10.5 PPC (which predates all the ARC stuff), but I'd still like to add some unit tests for it.

[colincornaby]

I think stuff like hsMessage could use the CF bridged versions.

Trying the run the existing client and code on a pre-ARC system is a whole... thing. I did the move to ARC because there just aren't many non-ARC developers out there - and it kept the code accessible. 10.5 era AppKit is also very different.

Are you attempting to use the existing Mac client on 10.5? Or are you writing a new shell?

Obj-C code that sits in the engine itself could be moved back to manual memory management. That would really be hsMessageBox, the keychain stuff doesn't care about ARC. I don't know if the code would work with Obj-C garbage collection. GC was rarely used but also promised to work without retains/releases.

The actual API used could also end up being a problem for 10.5. And I think the xib encoding has changed since the PPC era tools. It might be better if 10.5 PPC had it's own client - if you were thinking of using the Mac client.

[dpogue]

I'm pretty sure the current macOS client won't work with 10.5, so I haven't even tried it yet. I'm more interested in making sure the core engine stuff is able to work on 10.5 than the client bits right now.

IIRC the updates to pfPasswordStore for updating passwords cause it to use APIs that didn't exist in 10.5, and hsMessageBox doesn't currently work because it's using block syntax and @autoreleasepool that aren't supported by standard GCC.

[dpogue]

I've updated this to restore the Objective C category for plClient code, but implemented using the new hsDarwin methods under the hood.

[colincornaby]

Fun side quest - I let Xcode name the category, but I wanted to find out if there was convention.

The + format for categories IS actually defined convention: https://developer.apple.com/library/archive/documentation/General/Conceptual/DevPedia-CocoaCore/Category.html

[colincornaby]

One more thing I'll add - changes to the Mac code will probably be necessary, and I don't want to railroad any existing conventions. However - I'll also probably be a little conservative on changes for the time being because changes here may roll down to the Metal client and cause conflicts.

[Hoikas]

Is this good to go?

[dpogue]

Is this good to go?

IMO, yes

[colincornaby]

I don't have any more feedback. (Already approved.)

[dgelessus]

Should be okay from my side too. The one thing that I thought was a bug is actually fine. The other discussions I opened are about API organization, so they're not essential and/or can be addressed later if needed.