<html><head><meta http-equiv="Content-Type" content="text/html charset=utf-8"></head><body style="word-wrap: break-word; -webkit-nbsp-mode: space; -webkit-line-break: after-white-space;" class=""><br class=""><div><blockquote type="cite" class=""><div class="">On Mar 2, 2015, at 2:25 PM, Nico Weber <<a href="mailto:thakis@chromium.org" class="">thakis@chromium.org</a>> wrote:</div><br class="Apple-interchange-newline"><div class=""><div dir="ltr" class="">One more tweak: Apparently for `@interface A @end @class A;`, a lookup for A finds the @interface decl, not the @class redecl. </div></div></blockquote><div><br class=""></div><div>Yeah, I think this is a longstanding hack from when we starting tracking redeclaration chains for Objective-C class declarations (and @class started being an ObjCInterfaceDecl). We (ahem, I) didn’t hunt down all of the places where we were expecting name lookup to return the definition.</div><br class=""><blockquote type="cite" class=""><div class=""><div dir="ltr" class="">So for ObjCInterfaceDecls, the explicit loop is necessary – this patch adds it back for that case. With this change, I can build most of chrome with this warning enabled (and some declaration tweaks in chrome). (Only "most of" because the build is still running – no known bugs).</div></div></blockquote><div><br class=""></div><div>Okay. Everything else LGTM, I say “go for it!”.</div><div><br class=""></div><span class="Apple-tab-span" style="white-space:pre"> </span>- Doug</div><div><br class=""><blockquote type="cite" class=""><div class=""><div class="gmail_extra"><div class="gmail_quote">On Sun, Mar 1, 2015 at 7:52 PM, Nico Weber <span dir="ltr" class=""><<a href="mailto:thakis@chromium.org" target="_blank" class="">thakis@chromium.org</a>></span> wrote:<br class=""><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr" class=""><div class="gmail_extra"><div class="gmail_quote"><span class="">On Mon, Feb 2, 2015 at 10:26 PM, Douglas Gregor <span dir="ltr" class=""><<a href="mailto:dgregor@apple.com" target="_blank" class="">dgregor@apple.com</a>></span> wrote:<br class=""><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div style="word-wrap:break-word" class="">Hi Nico,<div class=""><br class=""><div class=""><span class=""><blockquote type="cite" class=""><div class="">On Jan 8, 2015, at 6:14 PM, Nico Weber <<a href="mailto:thakis@chromium.org" target="_blank" class="">thakis@chromium.org</a>> wrote:</div><br class=""><div class=""><div dir="ltr" class=""><div class="">Hi,</div><div class=""><br class=""></div><div class="">the Mac OS X and iOS SDKs add new functions in new releases. Apple recommends using the newest SDK and setting the deployment target to whatever old OS version one wants to support, and only calling new functions after checking that they are available at runtime.</div><div class=""><br class=""></div><div class="">In practice, we (Chromium) get this wrong. Others who support old OS X versions get this wrong too. Hence, we (Chromium) use a very old SDK and then manually declare new functions when we want to call them – this reduces the chance of us forgetting if they are available at runtime considerably, in practice. But using an old SDK has its problems – sometimes the frameworks check which SDK an app was linked against and only then activate bug fixes, and newer Xcodes don't ship include old SDKs.</div></div></div></blockquote><div class=""><br class=""></div></span><div class="">That’s an interesting approach to handling the availability problem; I hadn’t heard of it before, but I see the logic there.</div><span class=""><br class=""><blockquote type="cite" class=""><div class=""><div dir="ltr" class=""><div class="">Ideally, we could use a new SDK but get a warning when we use a new API without a manual redeclaration – this protects us against new APIs the same way using an old SDK does without the drawbacks that this brings.</div><div class=""><br class=""></div><div class="">The attached patch is a sketch how such a warning might work. How repulsive is this idea? Are there other approaches to this problem? If the basic idea is ok: </div></div></div></blockquote><div class=""><br class=""></div></span><div class="">This is a drastically different approach than I’d imagined. Whenever I’ve thought about this problem, I’ve always come back to some form of dataflow analysis that checks whether uses of “not-yet-introduced” API is used in a sane way: is it dominated by some check that implies the availability, e.g., a -respondsToSelector: check on a method with at least that availability, or checking whether “[NSFoo class]” is non-null when the class has availability. I suspect that’s the idea behind Deploymate (<a href="http://www.deploymateapp.com/" target="_blank" class="">http://www.deploymateapp.com</a>), although I’ve never used it, and it has the benefit that it should make idiomatic code (that does the right checking) just work.</div><div class=""><br class=""></div><div class="">It’s also a heck of a lot more work to implement than the approach you’re using.</div></div></div></div></blockquote><div class=""><br class=""></div></span><div class="">Right :-)</div><span class=""><div class=""> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div style="word-wrap:break-word" class=""><div class=""><div class=""><span class=""><div class=""><br class=""></div><blockquote type="cite" class=""><div class=""><div dir="ltr" class=""><div class="">Any comments on the implementation?</div></div></div></blockquote><br class=""></span></div><div class="">The implementation generally looks fine. One minor comment:</div><div class=""><br class=""></div><div class=""><div class="">+ case AR_NotYetIntroduced: {</div><div class="">+ // don't do this for enums, they can't be redeclared.</div><div class="">+ if (isa<EnumConstantDecl>(D) || isa<EnumDecl>(D))</div><div class="">+ break;</div><div class="">+ bool FoundRedecl = false;</div><div class="">+ for (Decl *Redecl = D->getMostRecentDecl(); Redecl && !FoundRedecl;</div><div class="">+ Redecl = Redecl->getPreviousDecl()) {</div><div class="">+ if (Redecl->getAttr<AvailabilityAttr>()->isInherited())</div><div class="">+ FoundRedecl = true;</div><div class="">+ }</div><div class="">+ if (!FoundRedecl)</div><div class="">+ S.EmitAvailabilityWarning(Sema::AD_Partial, D, Message, Loc,</div><div class="">+ UnknownObjCClass, ObjCPDecl,</div><div class="">+ ObjCPropertyAccess);</div><div class="">+ break;</div><div class="">+ }</div><div class=""><br class=""></div><div class="">Generally speaking, name lookup will always find the most recent declaration, so you might be able to skip the D->getMostRecentDecl() bit entirely and just check that the availability attribute was inherited.</div></div></div></div></blockquote><div class=""><br class=""></div></span><div class="">That worked, done.</div><div class=""><br class=""></div><div class="">I also had to add some explicit code for handling redeclarations in @interfaces, as these aren't modeled as redeclarations in the AST. I also added the property note used in the other availability warnings, and I added lots of tests.</div></div></div></div>
</blockquote></div><br class=""></div>
<span id="cid:1AA5C88D-19E6-41AE-BD42-78F157D34581@apple.com"><clang-redecl.diff></span></div></blockquote></div><br class=""></body></html>