> On Mar 20, 2015, at 11:07 AM, Nico Weber <tha...@chromium.org> wrote:
>
> On Fri, Mar 20, 2015 at 10:42 AM, Ted Kremenek <kreme...@apple.com
> <mailto:kreme...@apple.com>> wrote:
>
>> On Mar 20, 2015, at 8:35 AM, Nico Weber <tha...@chromium.org
>> <mailto:tha...@chromium.org>> wrote:
>>
>> On Fri, Mar 20, 2015 at 8:10 AM, Ted Kremenek <kreme...@apple.com
>> <mailto:kreme...@apple.com>> 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.
>
>
> 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.
_______________________________________________
cfe-commits mailing list
cfe-commits@cs.uiuc.edu
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
