<div dir="ltr"><div class="gmail_extra"><div class="gmail_quote">On Fri, Mar 20, 2015 at 8:10 AM, Ted Kremenek <span dir="ltr"><<a href="mailto:kremenek@apple.com" target="_blank">kremenek@apple.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div style="word-wrap:break-word">Hi Nico,<div><br></div><div>I'm really sorry, but this is the first time I saw this. When you first proposed the patch I was away from work for several weeks and wasn't reading email. I then missed most of this.</div><div><br></div><div>I honestly am very concerned about this approach. The problem is certainly well-motivated, but it feels like a really partial solution to the problem that doesn't provide any real safety. Looking back at the thread I see you and Doug discussed using dataflow analysis, which really seems like the right approach to me. Even some basic lexical analysis to check if a guard existed before use probably would have provided some reasonable checking, and I disagree with Doug that the dataflow approach was "a heck of a lot more work".</div><div><br></div><div>The thing I don't like about this approach is that as soon as you provide the redeclaration you lose all checking for uses of a method. Here are my concerns:</div><div><br></div><div>(1) You get a warning once for a method that is newer than your minimum deployment target regardless of whether or not you are using it safely.</div><div><br></div><div>(2) You silence that warning by adding the redeclaration. Then all future uses of that method that you introduce won't get a warning.</div><div><br></div><div>I don't see how this provides any real checking at all. Even if the first warning was valid in identify and unguarded case, you get no help from the compiler if you add the guard and do it incorrectly.</div><div><br></div><div>My concern here is not that this problem isn't important to solve. It's a very big problem. I am concerned that this approach causes users to clutter their code with redeclarations and it provides them no much safety checking at all. I know part of this was motivated by real issues you were seeing in Chrome, a large codebase that needs to run on multiple OS versions. Do you think this will be a practical, and useful approach, to solving that problem in practice on a codebase of that size?</div></div></blockquote><div><br></div><div>Hi Ted,</div><div><br></div><div>I agree that this is an imperfect solution. However, it's identical to our current approach of just building against an old SDK (10.6). This is what we currently do, and having to declare methods before using them does help in practice. However, the OS suppresses some bug fixes when linking against an old SDK, so we want to switch to a model where we use the newest SDK. This warning is supposed to give us the same level of safety as using an old SDK, without getting the drawbacks of an old SDK.</div><div><br></div><div>This isn't Chromium-specific either: I talked to the Firefox folks, and they currently build against an old SDK for the same reasons we do.</div><div><br></div><div>(Also note that this warning is off by default and not in -Wall.)</div><div><br></div><div>Nico</div><div> </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"><div><br></div><div>I'm very sorry to wade into this so late.</div><div><br></div><div>Ted</div><div><br><div><blockquote type="cite"><div><div class="h5"><div>On Mar 19, 2015, at 12:23 PM, Nico Weber <<a href="mailto:thakis@chromium.org" target="_blank">thakis@chromium.org</a>> wrote:</div><br></div></div><div><div><div class="h5"><div dir="ltr">r232750, thanks!<br></div><div class="gmail_extra"><br><div class="gmail_quote">On Tue, Mar 17, 2015 at 9:44 AM, Douglas Gregor <span dir="ltr"><<a href="mailto:dgregor@apple.com" target="_blank">dgregor@apple.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div style="word-wrap:break-word"><br><div><span><blockquote type="cite"><div>On Mar 2, 2015, at 2:25 PM, Nico Weber <<a href="mailto:thakis@chromium.org" target="_blank">thakis@chromium.org</a>> wrote:</div><br><div><div dir="ltr">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></div></span><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><span><br><blockquote type="cite"><div><div dir="ltr">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></div></span><div>Okay. Everything else LGTM, I say “go for it!”.</div><div><br></div><span style="white-space:pre-wrap"> </span>- Doug</div><div><br><blockquote type="cite"><div><div><div><div class="gmail_extra"><div class="gmail_quote">On Sun, Mar 1, 2015 at 7:52 PM, Nico Weber <span dir="ltr"><<a href="mailto:thakis@chromium.org" target="_blank">thakis@chromium.org</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><span>On Mon, Feb 2, 2015 at 10:26 PM, Douglas Gregor <span dir="ltr"><<a href="mailto:dgregor@apple.com" target="_blank">dgregor@apple.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div style="word-wrap:break-word">Hi Nico,<div><br><div><span><blockquote type="cite"><div>On Jan 8, 2015, at 6:14 PM, Nico Weber <<a href="mailto:thakis@chromium.org" target="_blank">thakis@chromium.org</a>> wrote:</div><br><div><div dir="ltr"><div>Hi,</div><div><br></div><div>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><br></div><div>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><br></div></span><div>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><br><blockquote type="cite"><div><div dir="ltr"><div>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><br></div><div>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><br></div></span><div>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">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><br></div><div>It’s also a heck of a lot more work to implement than the approach you’re using.</div></div></div></div></blockquote><div><br></div></span><div>Right :-)</div><span><div> </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"><div><div><span><div><br></div><blockquote type="cite"><div><div dir="ltr"><div>Any comments on the implementation?</div></div></div></blockquote><br></span></div><div>The implementation generally looks fine. One minor comment:</div><div><br></div><div><div>+ case AR_NotYetIntroduced: {</div><div>+ // don't do this for enums, they can't be redeclared.</div><div>+ if (isa<EnumConstantDecl>(D) || isa<EnumDecl>(D))</div><div>+ break;</div><div>+ bool FoundRedecl = false;</div><div>+ for (Decl *Redecl = D->getMostRecentDecl(); Redecl && !FoundRedecl;</div><div>+ Redecl = Redecl->getPreviousDecl()) {</div><div>+ if (Redecl->getAttr<AvailabilityAttr>()->isInherited())</div><div>+ FoundRedecl = true;</div><div>+ }</div><div>+ if (!FoundRedecl)</div><div>+ S.EmitAvailabilityWarning(Sema::AD_Partial, D, Message, Loc,</div><div>+ UnknownObjCClass, ObjCPDecl,</div><div>+ ObjCPropertyAccess);</div><div>+ break;</div><div>+ }</div><div><br></div><div>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><br></div></span><div>That worked, done.</div><div><br></div><div>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></div>
</div></div><span><clang-redecl.diff></span></div></blockquote></div><br></div></blockquote></div><br></div></div></div>
_______________________________________________<br>cfe-commits mailing list<br><a href="mailto:cfe-commits@cs.uiuc.edu" target="_blank">cfe-commits@cs.uiuc.edu</a><br><a href="http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits" target="_blank">http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits</a><br></div></blockquote></div><br></div></div></blockquote></div><br></div></div>