> On Mar 20, 2015, at 11:38 AM, Nico Weber <[email protected]> wrote: > > On Fri, Mar 20, 2015 at 11:28 AM, Ted Kremenek <[email protected] > <mailto:[email protected]>> wrote: > >> On Mar 20, 2015, at 11:07 AM, Nico Weber <[email protected] >> <mailto:[email protected]>> wrote: >> >> On Fri, Mar 20, 2015 at 10:42 AM, Ted Kremenek <[email protected] >> <mailto:[email protected]>> wrote: >> >>> On Mar 20, 2015, at 8:35 AM, Nico Weber <[email protected] >>> <mailto:[email protected]>> wrote: >>> >>> On Fri, Mar 20, 2015 at 8:10 AM, Ted Kremenek <[email protected] >>> <mailto:[email protected]>> wrote: >>> Hi Nico, >>> >>> 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. >>> >>> 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". >>> >>> 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: >>> >>> (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. >>> >>> (2) You silence that warning by adding the redeclaration. Then all future >>> uses of that method that you introduce won't get a warning. >>> >>> 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. >>> >>> 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? >>> >>> Hi Ted, >>> >>> 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. >>> >>> 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. >>> >>> (Also note that this warning is off by default and not in -Wall.) >>> >>> Nico >>> >> >> Hi Nico, >> >> My main concern about this warning, as is, is that it creates a workflow >> that castrates itself immediately and adds lots of ancillary boilerplate to >> the code whose purpose is mostly indiscernible to a reader. It also doesn't >> provide any real checking, and thus in my opinion creates a false sense of >> security. >> >> Again, this approach does help us in practice. I agree that it's not >> perfect, but it does work for us. > > I understand that it works for us, but it's not a great workflow in general. > It would be great if we had something that worked in general for OS X and iOS > developers. This feels, at least to me, very idiomatic to something that > might be adopted in a particular codebase. > >> >> People often get the actual checking wrong, and they may also use an >> unavailable API multiple times. >> >> I do think that we can take what you have and make something much better, >> and far more powerful, with not much effort. Here's a counterproposal: >> >> (a) Have the availability warning, as we have now, but delay issuing it >> until you can do a bit more analysis to validate if the use of the >> unavailable API is safe or not. >> >> (b) For each use of an unavailable API, walk up the lexical structure and >> look for 'if' guards for 'respondsToSelector'. If there is a >> 'respondsToSelector' check for the given potentially unavailable selector, >> then suppress the warning. >> >> (c) As a refinement, you can possibly generalize the 'respondsToSelector' >> check to imply that other unavailable selectors that have the same API >> availability would also be guarded against. This is potentially unsafe, >> however, as sometimes an API was internal Apple API in a previous OS, but >> this is an idiom that is widely followed. Actually, 'respondsToSelector' >> for just checking on potentially unavailable selector is unsafe for this >> reason. Regardless, I think the analysis would be pretty much the same. >> You can just walk the lexical structure and look for availability guards >> that imply a minimum OS availability. This seems very trivial to do; this >> is just an AST walk. You can also batch it up and do it efficiently by only >> lazily doing it if a potentially unavailable API is encountered. >> >> (d) As an escape hatch, provide a more localized way to suppress the warning >> other than suppressing all cases of the warning by adding a category. The >> category approach is completely non-obvious anyway, and from a readability >> perspective doesn't provide any insight that the warning is being suppressed >> by this bit of what appears to be redundant code. >> >> The motivation is that it is the same mechanism one would use when building >> against an old SDK. >> >> What do you think would be a better escape hatch syntax? > > I'm talking to process here, but we do have pragmas already for suppressing > warnings. Those can already be used to suppress warnings within blocks of > code. That idea could be extended to suppress the warning for specific > selectors, or at least selectors with in an availability equivalence class > (so to speak). > > Note that this approach could extend to not just selectors, but any API. > > Brainstorming a bit: > #pragma assume_availability(mac, 10.8) > …actually, that all I've got :-P
This seems reasonable, but to be clear I'd have it bracketed with two pragmas. Everything within that bracket would follow that assumption. For example: #pragma clang assume_availability(macosx, 10.8) begin ... #pragma clang assume_availability(macosx, 10.8) end Some other considerations: - the name should follow exactly what is spelled in the availability attribute for the platform name. - what about code that runs on different platforms? Straw man: #pragma clang assume_availability(macosx, 10.8) begin #pragma clang assume_availability(ios, 8.0) begin ... #pragma clang assume_availability(ios, 8.0) end #pragma clang assume_availability(macosx, 10.8) end or something more condense? I assume if you are running with this idea that it appeals to you. > > >> >> >> This counterproposal doesn't require dataflow analysis, and provides a >> strong model where actual checking is done. It also doesn't create a >> workflow that castrates itself upon silencing the first warning for an >> unavailable selector. >> >> Did you consider an approach like this? >> >> Yes. It's not sufficient. Some of our classes do calls to partial methods >> unconditionally, and we only instantiate these classes after checking that >> the OS is new enough (else we build a dummy object) with the same interface. > > That's a very good point. > > The suggestion I made above (with pragmas) could solve that problem, and > would allow you to be more declarative about the actual availability you > expect some code to be executed. I think that would communicate far more > intent than just adding categories that redeclare methods, and it gives you > the flexibility to decide at what granularity you want to suppress the > warning. > > Ok, for next steps: > > 1.) I'll try to come up with a patch that suppresses the current warning in > if (respondsToSelector) blocks for objc message sends, and in if (CFWeakFun) > for C functions. Sounds good. > 2.) I'll wait a bit for folks to brainstorm how exactly the pragma will look, > and once that has settled use that as a signal instead of redeclarations. Sounds great Nico.
_______________________________________________ cfe-commits mailing list [email protected] http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
