[PATCH] D45699: [Availability] Improve availability to consider functions run at load time

[Erik Pilkington via Phabricator via cfe-commits]

erik.pilkington added a comment.

Hi Steven, thanks for working on this!



================
Comment at: include/clang/Basic/DiagnosticSemaKinds.td:2916
+  "ignoring availability attribute %select{on '+load' method|"
+  "with constructor attribute|with desctructor attribute}0">,
+  InGroup<Availability>;
----------------
typo: destructor


================
Comment at: lib/Sema/SemaDecl.cpp:9134-9151
+  // Diagnose availability attributes. Availability cannot be used on functions
+  // that are run during load/unload.
+  for (const auto& attr: NewFD->attrs()) {
+    if (!isa<AvailabilityAttr>(attr))
+      continue;
+
+    if (NewFD->hasAttr<ConstructorAttr>()) {
----------------
Shouldn't this be in ProcessDeclAttributeList in SemaDeclAttr.cpp?


================
Comment at: lib/Sema/SemaDecl.cpp:9136
+  // that are run during load/unload.
+  for (const auto& attr: NewFD->attrs()) {
+    if (!isa<AvailabilityAttr>(attr))
----------------
Can't this just be: `if (NewFD->hasAttr<AvailabilityAttr>())`?


================
Comment at: lib/Sema/SemaDeclAttr.cpp:6828
+    if (const auto *MethodD = dyn_cast<ObjCMethodDecl>(Ctx))
+      if (MethodD->isClassMethod() && MethodD->getNameAsString() == "load")
+        return true;
----------------
getNameAsString is deprecated, maybe you should get this from MethodD->getSelector()?


================
Comment at: lib/Sema/SemaDeclObjC.cpp:4737-4738
 
+  // + load method cannot have availability attributes. It has to be the same
+  // as deployment target.
+  for (const auto& attr: ObjCMethod->attrs()) {
----------------
Second sentence might read better as "It get called on startup, so it has to have the availability of the deployment target".


================
Comment at: lib/Sema/SemaDeclObjC.cpp:4739-4740
+  // as deployment target.
+  for (const auto& attr: ObjCMethod->attrs()) {
+    if (!isa<AvailabilityAttr>(attr))
+      continue;
----------------
This should also be `if (ObjCMethod->hasAttr<AvailabilityAttr>)`


================
Comment at: lib/Sema/SemaDeclObjC.cpp:4744
+    if (ObjCMethod->isClassMethod() &&
+        ObjCMethod->getNameAsString() == "load") {
+      Diag(attr->getLocation(), diag::warn_availability_on_static_initializer)
----------------
Same comment as above!


================
Comment at: test/SemaObjC/unguarded-availability.m:334
+ at implementation HasStaticInitializer1
++ (void)load {
+  func_10_11(); // expected-warning{{'func_10_11' is only available on macOS 10.11 or newer}} expected-note{{enclose 'func_10_11' in an @available check to silence this warning}}
----------------
Could you add a test that `+(void)load: (int)x;` still works? It should still accept availability attributes, right?


Repository:
  rC Clang

https://reviews.llvm.org/D45699