jspahrsummers / libextobjc

A Cocoa library to extend the Objective-C programming language.
MIT License
4.53k stars 461 forks source link

Added extra check to keypath macro #110

Closed k06a closed 8 years ago

k06a commented 8 years ago

Thanks @nlutsenko for idea implementation: https://github.com/facebook/KVOController/pull/95

nlutsenko commented 8 years ago

Suggestion: Do not use kp for variable name, but rather use extobjckeypath or __extobjckeypath__ or something that has the non-ambiguous and unique name. If anything on the local context has kp as a variable name - there is a good chance you will get a shadowing variable warning. THat's the reason I used fbkvokeypath which I hope is unique enough.

jspahrsummers commented 8 years ago

This is a good idea for a change, but I'm not a fan of adding a runtime check to a macro which is supposed to increase compile-time safety.

I'd like to think of a way to statically detect a single key. I'll try thinking about it, but let me know if you have any ideas too.

k06a commented 8 years ago

@jspahrsummers, here is compilation result with optimization enabled:

NSLog(@"%s", keypath(self.window.frame));

=>

    .loc    2 32 5                  @ /Users/k06a/Projects/Test/AppDelegate.m:32:5
    movw    r1, :lower16:(L_.str3-(LPC0_1+4))
Ltmp1:
    movt    r1, :upper16:(L_.str3-(LPC0_1+4))
    movw    r0, :lower16:(L__unnamed_cfstring_2-(LPC0_2+4))
LPC0_1:
    add r1, pc
    movt    r0, :upper16:(L__unnamed_cfstring_2-(LPC0_2+4))
LPC0_2:
    add r0, pc
    adds    r1, #5
    blx _NSLog

and this:

#define KEYPATH(PATH) \
    (((void)(NO && ((void)PATH, NO)), \
    ({ char *kp = strchr(# PATH, '.'); NSCAssert(kp, @"Provided key path is invalid."); kp + 1; })))

NSLog(@"%s", KEYPATH(self.window.frame));

=>

.loc    2 32 5                  @ /Users/k06a/Projects/Test/AppDelegate.m:32:5
    movw    r1, :lower16:(L_.str3-(LPC0_1+4))
Ltmp1:
    movt    r1, :upper16:(L_.str3-(LPC0_1+4))
    movw    r0, :lower16:(L__unnamed_cfstring_2-(LPC0_2+4))
LPC0_1:
    add r1, pc
    movt    r0, :upper16:(L__unnamed_cfstring_2-(LPC0_2+4))
LPC0_2:
    add r0, pc
    adds    r1, #5
    blx _NSLog

Looks like this is exactly the same assembly when compiling with Release scheme instead of Debug.

nlutsenko commented 8 years ago

Yup, because it's NSCAssert, which gets removed if using the release scheme, as it has NS_BLOCK_ASSERTIONS enabled (I hope I am right about the name).

k06a commented 8 years ago

Since it makes no difference in runtime, I see no problem to use this solution :)

nlutsenko commented 8 years ago

Yup, there is a big difference in development though... Hopefully no developer ships their stuff without running it at least once, right?

k06a commented 8 years ago

@jspahrsummers are you planning to merge it?

jspahrsummers commented 8 years ago

Looks good. Sorry, this was before GitHub notified of PR changes, I think. 😅