[PATCH] D23003: [ObjC Availability] Warn upon unguarded use of partially available declaration

[Manman Ren via cfe-commits]

manmanren added a comment.

Hi Erik,

Thanks for working on this! It is great to see these patches coming.

Manman


================
Comment at: include/clang/Sema/Sema.h:9608
@@ -9604,1 +9607,3 @@
 
+  /// \brief Whether we should emit an availability diagnostic for \c D.
+  bool ShouldDiagnoseAvailabilityOfDecl(
----------------
Can you explain the inputs and outputs?

================
Comment at: lib/Sema/SemaDeclAttr.cpp:6517
@@ +6516,3 @@
+
+/// \brief This class implements -Wunguarded-availability.
+class DiagnoseUnguardedAvailability
----------------
Can you add a high level description of what this class is doing to issue unguarded diagnostics?

================
Comment at: lib/Sema/SemaDeclAttr.cpp:6540
@@ +6539,3 @@
+
+  bool VisitTypeLoc(TypeLoc TL);
+
----------------
Do we cover all usage of decls with the above three member functions?

================
Comment at: lib/Sema/SemaDeclAttr.cpp:6552
@@ +6551,3 @@
+
+  if (SemaRef.ShouldDiagnoseAvailabilityOfDecl(D, nullptr, false, ObjCPDecl,
+                                               ContextVersion, nullptr, AD)) {
----------------
It is easier to read if you add comment for the boolean argument.

================
Comment at: lib/Sema/SemaDeclAttr.cpp:6554
@@ +6553,3 @@
+                                               ContextVersion, nullptr, AD)) {
+    // All other diagnostic kinds have already been handled.
+    if (AD != Sema::AD_Partial)
----------------
Can you mention where they are handled?

================
Comment at: lib/Sema/SemaExpr.cpp:110
@@ -113,1 +109,3 @@
+    VersionTuple ContextVersion, std::string *Message,
+    Sema::AvailabilityDiagnostic &AD) {
 
----------------
This looks like a refactoring of DiagnoseAvailabilityOfDecl. Is it possible to commit as a NFC patch?

================
Comment at: test/SemaObjC/unguarded-availability.m:93
@@ +92,3 @@
+  label2:
+    goto label1; // expected-error{{cannot jump from this goto statement to its label}}
+  }
----------------
Should we emit error here? Or should we just ignore the tighter availability scope?


https://reviews.llvm.org/D23003