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. > 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? > > 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. I agree that it's a nice addition to not emit this warning in places that _are_ protected by respondsToSelector. > > Ted > > > >
_______________________________________________ cfe-commits mailing list [email protected] http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
