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