This is an archive of the discontinued LLVM Phabricator instance.

Introduce -Wunguarded-availability-new, which is like -Wunguarded-availability, except that it's enabled by default for new deployment targets
ClosedPublic

Authored by arphaman on Jun 15 2017, 5:56 PM.

Details

Summary

This patch adds a new warning flag called -Wunguarded-availability-new. If -Wunguarded-availability is off, this warning only warns about uses of APIs that have been introduced in macOS >= 10.13, iOS >= 11, watchOS >= 4 and tvOS >= 11. This warning is on by default. We decided to use this kind of solution as we didn't want to turn on -Wunguarded-availability by default, as we didn't want our users to get warnings about uses of old APIs in their existing projects.

Diff Detail

Repository
rL LLVM

Event Timeline

arphaman created this revision.Jun 15 2017, 5:56 PM
lib/Sema/SemaDeclAttr.cpp
7315 ↗(On Diff #102766)

This assert seems a bit redundant, no?

7350 ↗(On Diff #102766)

There is a version of this for decls that aren't referenced in a function, such as:

typedef int __attribute__((availability(macos, introducced=1000))) new_int;
new_int x; // warn

// Also:
struct S {
  new_int x; // warn
};

-Wunguarded-availability-new should also work with this, right? If we do want to support that, maybe we should add this check to ShouldDiagnoseAvailabilityOfDecl(), which is called by both paths.

arphaman updated this revision to Diff 102891.Jun 16 2017, 3:21 PM
arphaman marked 2 inline comments as done.

Remove the assert and support the other partial availability warnings.

lib/Sema/SemaDeclAttr.cpp
6944 ↗(On Diff #102891)

Why did you remove the AR_Unavailable checking?

7021 ↗(On Diff #102891)

I think checking Diags::isIgnored is fairly expensive, maybe we should swap the operands to the '&&' so that it is only done if necessary?

arphaman added inline comments.Jun 16 2017, 4:17 PM
lib/Sema/SemaDeclAttr.cpp
6944 ↗(On Diff #102891)

Ooops, that was a mistake

arphaman updated this revision to Diff 102902.Jun 16 2017, 4:36 PM
arphaman marked an inline comment as done.

Swap the checking operators and bring back the accidentally deleted code.

lib/Sema/SemaDeclAttr.cpp
7031 ↗(On Diff #102902)

Sorry to keep this going so long, but why are we even checking isIgnored? The only difference it could make in whether we emit a diagnostic is if both: -Wunguarded-availability and -Wno-unguarded-availability-new are passed in, which seems like it would never happen, right? Even if somebody did pass that in, it seems reasonable to warn on old stuff but not new stuff. Maybe I'm missing something here?

arphaman added inline comments.Jun 19 2017, 10:18 AM
lib/Sema/SemaDeclAttr.cpp
7031 ↗(On Diff #102902)

Right, it's to handle the -Wunguarded-availability -Wno-unguarded-availability-new case. Your argument makes sense though, we could allow -Wunguarded-availability -Wno-unguarded-availability-new where we warn on old APIs. Although that still seems kinda weird to me. Maybe @dexonsmith has an opinion about this?

dexonsmith added inline comments.Jun 21 2017, 5:42 PM
lib/Sema/SemaDeclAttr.cpp
7031 ↗(On Diff #102902)

I don't think the exact behaviour of -Wunguarded-availability -Wno-unguarded-availability-new is terribly important. I'm fine either way.

But, having a predictable command-line interface is nice, and I'd expect that combination to show diagnostics only for old APIs.

arphaman updated this revision to Diff 103577.Jun 22 2017, 7:41 AM

Update the logic for -Wunguarded-availability -Wno-unguarded-availability-new so that it only warns for the old APIs.

erik.pilkington accepted this revision.Jun 22 2017, 8:09 AM

LGTM, thanks for working on this!

This revision is now accepted and ready to land.Jun 22 2017, 8:09 AM
This revision was automatically updated to reflect the committed changes.