On Fri, Mar 20, 2015 at 11:28 AM, Ted Kremenek <[email protected]> wrote:
> > On Mar 20, 2015, at 11:07 AM, Nico Weber <[email protected]> wrote: > > On Fri, Mar 20, 2015 at 10:42 AM, Ted Kremenek <[email protected]> wrote: > >> >> On Mar 20, 2015, at 8:35 AM, Nico Weber <[email protected]> wrote: >> >> On Fri, Mar 20, 2015 at 8:10 AM, Ted Kremenek <[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 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. 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 good?
_______________________________________________ cfe-commits mailing list [email protected] http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
