Support ARC #1

Open
wants to merge 6 commits into
from

5 participants

@mbaltaks

I have not done extensive testing, but this patch does seem to work under ARC in my current project, for what testing I've done.

@boredzo

This reverts Rachel's fix to the format string (which should be %lu, but %ld is closer than %d).

Owner

@rudyrichter: I've already applied that change in the upstream source. https://github.com/boredzo/iso-8601-date-formatter/blob/master/ISO8601DateFormatter.m#L750 @mbaltaks's version is behind on that (and I don't know how easy it would be to reintegrate them at this point).

@boredzo

It looks really weird to have code that releases the array (under MRC), then code that sends a message to it. Should be the other way, just for non-weirdness.

@boredzo

This reverts the other of Rachel's fixes.

@mbaltaks

My mistake, for some reason I didn't pick that up previously. I've sorted that out, and used the compiler recommendation for %u in the format string.

@boredzo
Owner

I added -fobjc-arc in the Makefile and got this:

make test                       %~/Projects/@reusable/iso-8601-date-formatter-mbaltaks(0)
/usr/bin/clang -std=c99 -fobjc-arc -g -Werror -Wmissing-field-initializers -Wreturn-type -Wmissing-braces -Wparentheses -Wswitch -Wunused-function -Wunused-label -Wunused-variable -Wunused-value -Wshadow -Wsign-compare -Wnewline-eof -Wshorten-64-to-32 -Wundeclared-selector -Wmissing-prototypes -Wformat -Wunknown-pragmas   -c -o testparser.o testparser.m
testparser.m:5:2: error: 'NSAutoreleasePool' is unavailable: not available in automatic
      reference counting mode
        NSAutoreleasePool *pool = [NSAutoreleasePool new];
        ^
/System/Library/Frameworks/Foundation.framework/Headers/NSAutoreleasePool.h:8:12: note: 
      declaration has been explicitly marked unavailable here
@interface NSAutoreleasePool : NSObject {
           ^
testparser.m:5:29: error: 'NSAutoreleasePool' is unavailable: not available in automatic
      reference counting mode
        NSAutoreleasePool *pool = [NSAutoreleasePool new];
                                   ^
/System/Library/Frameworks/Foundation.framework/Headers/NSAutoreleasePool.h:8:12: note: 
      declaration has been explicitly marked unavailable here
@interface NSAutoreleasePool : NSObject {
           ^
testparser.m:15:37: error: 'autorelease' is unavailable: not available in automatic
      reference counting mode
        ISO8601DateFormatter *formatter = [[[ISO8601DateFormatter alloc] init] ...
                                           ^
/System/Library/Frameworks/Foundation.framework/Headers/NSObject.h:37:1: note: 
      declaration has been explicitly marked unavailable here
- (id)autorelease NS_AUTOMATED_REFCOUNT_UNAVAILABLE;
^
testparser.m:15:37: error: ARC forbids explicit message send of 'autorelease'
  ...*formatter = [[[ISO8601DateFormatter alloc] init] autorelease];
                   ^                                   ~~~~~~~~~~~
testparser.m:25:3: error: 'release' is unavailable: not available in automatic reference
      counting mode
        [pool release];
         ^
/System/Library/Frameworks/Foundation.framework/Headers/NSObject.h:36:1: note: 
      declaration has been explicitly marked unavailable here
- (oneway void)release NS_AUTOMATED_REFCOUNT_UNAVAILABLE;
^
testparser.m:25:3: error: ARC forbids explicit message send of 'release'
        [pool release];
         ^    ~~~~~~~
6 errors generated.
make: *** [testparser.o] Error 1

Similar errors result in all of the other test-program files, which have not been ARCified or dual-moded. (Ideally, all of the files should be dual-mode, so that the parser and unparser can be regularly tested in either mode.)

@mbaltaks

Ok, I've not looked into running with and without ARC automatically, but if you manually add the compiler flag it all seems to look just as it does without the flag. The test output is quite verbose though, and there is no output from the time trial so I may be misunderstanding something there.

@blakewatters

Can we not just drop non-ARC support in the next version instead of doing this conditional compilation hackery?

@billinghamj

What's the state on this?

@boredzo
Owner

The state is:

  • This patch can't be automatically merged. Enough changes have happened and will happen that it'll probably be best/easiest to redo it. I'm just treating this as an issue ticket at this point.
  • There are still people using the ISO 8601 Date Formatter who support 32-bit OS X, and so cannot have ARC code in their projects—it must all use MRC. The biggest ones are Growl, who are maintaining 32-bit support in their framework, and Karelia. I intend to support this through at least the next release, and my current plan (subject to change) is to go ARC after then.
  • Also, ARC and other whole-project reworks are on hold until test coverage is at least a lot closer to 100%. There are still significant parts of the code that are not yet tested.

If anybody wants to help ARC happen in this project sooner, the best way is to contribute test cases. Coveralls will tell you what's currently not tested; here's the main source file as of the current latest build for anyone who's interested.

I should note that there are some test cases that have been removed because they fail, and some ported test-monster cases that fail. IIRC, I stubbed these out because they couldn't be fixed without the sort of major refactoring that I'm leaving until I have more test cases.

You're welcome to write test cases that fail. If they can't be fixed without major refactoring, I'll keep them on a separate branch.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment