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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
