Remove support for 10.7 #74
Conversation
I also fixed some linker errors that appeared when switching to 10.8 deployment target. Note that my system is 10.12, so I can’t _fully_ test 10.8 compatibility.
|
It looks like the comment saying that macOS 10.7 doesn't allow you to add a pointer past the end of the array still applies at least in 10.9, given that's what Travis CI is running the tests with. It compiles and tests fine on my 10.12 machine though. |
| @@ -203,15 +203,6 @@ - (void)dealloc | |||
| { | |||
| [_db close]; | |||
| }); | |||
|
|
|||
| #if !(TARGET_OS_IPHONE) | |||
There was a problem hiding this comment.
Should change to #if defined(GNUSTEP)
and the start of the comment should be changed to We are using deployment target 10.8, so ARC will manage libdispatch objects automatically. For GNUstep....
| * | ||
| * For referring object graph contexts, when unloaded or finalized, the | ||
| * deallocation will trigger their removal of their inner objects from the hash | ||
| * tables in the cache on 10.8 or iOS 6 or higher, but not on 10.7. |
There was a problem hiding this comment.
I'd keep this comment, but change the end to
10.8 or iOS 6 or higher, which we require.
| @@ -124,14 +124,6 @@ - (void)dealloc | |||
| [db_ close]; | |||
| db_ = nil; | |||
| }); | |||
|
|
|||
| #if !(TARGET_OS_IPHONE) | |||
There was a problem hiding this comment.
Should change to #if defined(GNUSTEP)
and the start of the comment should be changed to We are using deployment target 10.8, so ARC will manage libdispatch objects automatically. For GNUstep....
| dotFilePath, | ||
| pdfPath] UTF8String]); | ||
| [NSTask launchedTaskWithLaunchPath:executablePath | ||
| arguments:@[@"-Tpdf", dotFilePath, pdfPath]]; |
There was a problem hiding this comment.
Missing the -o param and waitUntilExit:
NSTask *task = [NSTask launchedTaskWithLaunchPath:executablePath
arguments:@[@"-Tpdf", dotFilePath, @"-o", pdfPath]];
[task waitUntilExit];
Add back GNUstep compatibility, make requested changes to comments, and fix Graphviz task execution.
|
@ericwa Well, my plan was to wait to get the test suite working again on GNUstep, before removing any 10.7 support. Since I don't have the time to fix CO on GNUstep currently, it might be simpler to move forward and remove 10.7 support even if this causes us some extra work on GNUstep later. I might have a bit more time next week to get CO working on GNUstep again. Not sure yet though. My main worry was about the zeroing weak reference support on GNUstep. For example, if we remove |
| executablePath, | ||
| dotFilePath, | ||
| pdfPath] UTF8String]); | ||
| NSTask *task = [NSTask launchedTaskWithLaunchPath:executablePath |
There was a problem hiding this comment.
A space is missing between launchedTaskWithLaunchPath: and executablePath. Same right after arguments:.
| @@ -91,8 +91,6 @@ - (instancetype)init | |||
| - (void)dealloc | |||
| { | |||
| #ifdef DELETE_STORE_AFTER_EACH_TEST_METHOD | |||
There was a problem hiding this comment.
DELETE_STORE_AFTER_EACH_TEST_METHOD should now be defined by default or removed. @ericwa, what's your take on this?
| @@ -32,14 +28,7 @@ - (void)addReferringObject: (COObject *)aReferrer | |||
|
|
|||
| if (referringObjects == nil) | |||
| { | |||
| // FIXME: If we don't ditch 10.7 support, we need a reverse mapping | |||
There was a problem hiding this comment.
This needs to be checked, I'm not 100% sure about what follows… I think we should either keep this comment somewhere or remove _referringObjectToPaths. If this FIXME is fully correct, _referringObjectToPaths is probably only needed for -removePath: to work around missing support for zeroing weak references.
@ericwa, any comments on this point?
I also fixed some linker errors that appeared when switching to 10.8 deployment target. Note that my system is 10.12, so I can’t fully test 10.8 compatibility.
See: #61