[PATCH] D73755: [objc_direct] Small updates to help with adoption.

[Erik Pilkington via Phabricator via cfe-commits]

erik.pilkington added inline comments.


================
Comment at: clang/include/clang/Basic/DiagnosticSemaKinds.td:1005-1006
 def err_objc_direct_duplicate_decl : Error<
-  "%select{|direct }0method declaration conflicts "
-  "with previous %select{|direct }1declaration of method %2">;
+  "%select{|direct }0%select{method|property}1 declaration conflicts "
+  "with previous %select{|direct }2declaration of method %3">;
 def err_objc_direct_impl_decl_mismatch : Error<
----------------
This diagnostics reads a bit awkward when a property is conflicting with a property, since the the first half of the diagnostic is talking about the property declaration, but the second half is implicitly referring to the generated getter/setter.


================
Comment at: clang/include/clang/Basic/DiagnosticSemaKinds.td:1023
+def err_objc_direct_dynamic_property : Error<
+  "property was previously declared to be direct">;
 
----------------
I think this might be more clear as: "direct property cannot be @dynamic"


================
Comment at: clang/lib/Sema/SemaExprObjC.cpp:3032
+      }
+      Builder.~DiagnosticBuilder();
       Diag(Method->getLocation(), diag::note_direct_method_declared_at)
----------------
I believe its UB to call a destructor twice. Please use a compound statement to control where the dtor will be called here.


================
Comment at: clang/lib/Sema/SemaExprObjC.cpp:3046
+      }
+      Builder.~DiagnosticBuilder();
       Diag(Method->getLocation(), diag::note_direct_method_declared_at)
----------------
ditto


================
Comment at: clang/lib/Sema/SemaObjCProperty.cpp:1630-1637
+  if (PIDecl->getPropertyImplementation() == ObjCPropertyImplDecl::Dynamic &&
+      PIDecl->getPropertyDecl() &&
+      PIDecl->getPropertyDecl()->isDirectProperty()) {
+    Diag(PropertyLoc, diag::err_objc_direct_dynamic_property) << PropertyId;
+    Diag(PIDecl->getPropertyDecl()->getLocation(),
+         diag::note_previous_declaration);
+    return nullptr;
----------------
Would you mind adding a test for this diagnostic? It looks like err_objc_direct_dynamic_property doesn't take a parameter too, so there isn't any reason to do `<< PropertyId` unless you add one.


================
Comment at: clang/lib/Sema/SemaObjCProperty.cpp:2436
+  if (!GetterMethod) {
+    if (const ObjCCategoryDecl *CatDecl = dyn_cast<ObjCCategoryDecl>(CD)) {
+      auto *ExistingGetter = CatDecl->getClassInterface()->lookupMethod(
----------------
Just to be clear: so we need this check because the getter/setters of a property declared in a category aren't considered to override any getters/setters of the extended class, so the existing CheckObjCMethodOverrides checks below don't work?


================
Comment at: clang/test/SemaObjC/method-direct.m:51
 
-__attribute__((objc_root_class, objc_direct_members)) // expected-error {{'objc_direct_members' attribute only applies to Objective-C implementation declarations and Objective-C containers}}
- at interface SubDirectFail : Root
----------------
Can you leave this in so we have a test that has this attribute on an `@interface`?


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D73755/new/

https://reviews.llvm.org/D73755