[PATCH] D39913: [ObjC] warn about availability attributes missing from a method's declaration when they're specified for a method's definition

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman requested changes to this revision.
aaron.ballman added inline comments.
This revision now requires changes to proceed.


================
Comment at: include/clang/Basic/DiagnosticSemaKinds.td:2921
+def warn_availability_on_implementation_not_interface : Warning<
+  "method declaration is missing an availability attribute for "
+  "%0 that is specified in the definition">,
----------------
Please quote 'availability' in the diagnostic wording. Same for the note.


================
Comment at: include/clang/Basic/DiagnosticSemaKinds.td:2925
+def warn_deprecated_on_implementation_not_interface : Warning<
+  "method declaration is missing a deprecated attribute that is specified in "
+  "the definition">,
----------------
Please quote 'deprecated' in the diagnostic wording. Same for the note.

One concern I have about this diagnostic is that it suggests this will happen for *all* deprecated attributes when it does not.


================
Comment at: include/clang/Sema/Sema.h:2406-2407
+  /// This will warn on any missing clauses in the declaration.
+  void checkMissingAvailabilityClausesInDeclaration(NamedDecl *Original,
+                                                    NamedDecl *Implementation);
+
----------------
These should be `const` pointers.

Does this *only* warn on missing clauses, or does it also warn on  conflicting clauses as the first sentence suggests?


================
Comment at: lib/Sema/SemaDecl.cpp:3581-3582
+    if (Imp->getClassInterface() == DC ||
+        (isa<ObjCCategoryDecl>(DC) &&
+         cast<ObjCCategoryDecl>(DC)->IsClassExtension() &&
+         Imp->getClassInterface() ==
----------------
Please do not use `isa<>` followed by `cast<>` (same below). Rather than use this complex if statement, it might be better to split things out into local variables.


================
Comment at: lib/Sema/SemaDeclAttr.cpp:2286
+  llvm::SmallPtrSet<const IdentifierInfo *, 4> MissingPlatformAttributes;
+  for (auto *Def : Implementation->specific_attrs<AvailabilityAttr>()) {
+    bool MissingIntroduced = !Def->getIntroduced().empty();
----------------
`const auto *`


================
Comment at: lib/Sema/SemaDeclAttr.cpp:2291
+    bool MissingUnavailable = Def->getUnavailable();
+    for (auto *Decl : Original->specific_attrs<AvailabilityAttr>()) {
+      if (Def->getPlatform() != Decl->getPlatform())
----------------
`const auto *`


================
Comment at: lib/Sema/SemaDeclAttr.cpp:2295
+      MissingIntroduced =
+          MissingIntroduced ? Decl->getIntroduced().empty() : false;
+      MissingDeprecated =
----------------
ahatanak wrote:
> I feel like using "if" is easier to understand than a conditional operator, but it's up to you:
> 
> ```
> if (MissingIntroduced)
>   MissingIntroduced = Decl->getIntroduced().empty();
> ```
The pattern we usually see is: `MissingIntroduced = MissingIntroduced && Decl->getIntroduced().empty();`


Repository:
  rL LLVM

https://reviews.llvm.org/D39913