r196314 - [objc] Introduce attribute 'objc_designated_initializer'.

[Aaron Ballman]

On Thu, Dec 5, 2013 at 3:35 PM, Argyrios Kyrtzidis <akyrtzi at gmail.com> wrote:
> Thanks for reviewing! Comments inline.
>
> On Dec 3, 2013, at 1:38 PM, Aaron Ballman <aaron at aaronballman.com> wrote:
>
> On Tue, Dec 3, 2013 at 4:11 PM, Argyrios Kyrtzidis <akyrtzi at gmail.com>
> wrote:
>
> Author: akirtzidis
> Date: Tue Dec  3 15:11:25 2013
> New Revision: 196314
>
> URL: http://llvm.org/viewvc/llvm-project?rev=196314&view=rev
> Log:
> [objc] Introduce attribute 'objc_designated_initializer'.
>
> It only applies to methods of init family in an interface declaration.
>
> Added:
>    cfe/trunk/test/SemaObjC/attr-designated-init.m
> Modified:
>    cfe/trunk/include/clang/Basic/Attr.td
>    cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td
>    cfe/trunk/lib/Sema/SemaDeclAttr.cpp
>
> Modified: cfe/trunk/include/clang/Basic/Attr.td
> URL:
> http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/Basic/Attr.td?rev=196314&r1=196313&r2=196314&view=diff
> ==============================================================================
> --- cfe/trunk/include/clang/Basic/Attr.td (original)
> +++ cfe/trunk/include/clang/Basic/Attr.td Tue Dec  3 15:11:25 2013
> @@ -684,6 +684,11 @@ def ObjCSuppressProtocol : InheritableAt
>   let Args = [IdentifierArgument<"Protocol">];
> }
>
> +def ObjCDesignatedInitializer : Attr {
> +  let Spellings = [GNU<"objc_designated_initializer">];
> +  let Subjects = SubjectList<[ObjCMethod], ErrorDiag>;
>
>
> Considering how simple the semantic checks are, I think a
> SubsetSubject is more appropriate.  Something like:
>
> def ObjCInitFamily : SubsetSubject<ObjCMethod, [{S->getMethodFamily()
> == OMF_init}]>;
>
> def ObjCInterfaceDeclContext : SubsetSubject<ObjCMethod,
> [{isa<ObjCInterfaceDecl>(S->getDeclContext())}]>;
>
> +}
> +
> def Overloadable : Attr {
>   let Spellings = [GNU<"overloadable">];
>   let Subjects = SubjectList<[Function], ErrorDiag>;
>
> Modified: cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td
> URL:
> http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td?rev=196314&r1=196313&r2=196314&view=diff
> ==============================================================================
> --- cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td (original)
> +++ cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td Tue Dec  3 15:11:25
> 2013
> @@ -2427,6 +2427,12 @@ def warn_objc_requires_super_protocol :
> def note_protocol_decl : Note<
>   "protocol is declared here">;
>
> +// objc_designated_initializer attribute diagnostics.
> +def err_attr_objc_designated_not_init_family : Error<
> +  "'objc_designated_initializer' only applies to methods of the init
> family">;
> +def err_attr_objc_designated_not_interface : Error<
> +  "'objc_designated_initializer' only applies to methods of interface
> declarations">;
>
>
> Both of these should be attached to the existing diagnostics applying
> to attribute subjects (warn_attribute_wrong_decl_type and
> err_attribute_wrong_decl_type). When switching over to the subset
> subjects, you need to supply a custom error diagnostic argument
> anyway. Don't forget to update AttributeList.h with the new enumerant.
>
>
> Is it worth adding all that stuff to the tablegen setup just for one
> attribute ?
> I can see it more justified if there's at least another attribute that is
> going to use it.

Generally speaking, yes, because 1) less diagnostic bloat, and 2) it
lets you use the streamlined semantic checking, which is less error
prone. The goal is to push as much into tablegen as possible.
>
>
> +
> def err_ns_bridged_not_interface : Error<
>   "parameter of 'ns_bridged' attribute does not name an Objective-C class">;
>
>
> Modified: cfe/trunk/lib/Sema/SemaDeclAttr.cpp
> URL:
> http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Sema/SemaDeclAttr.cpp?rev=196314&r1=196313&r2=196314&view=diff
> ==============================================================================
> --- cfe/trunk/lib/Sema/SemaDeclAttr.cpp (original)
> +++ cfe/trunk/lib/Sema/SemaDeclAttr.cpp Tue Dec  3 15:11:25 2013
> @@ -3713,6 +3713,28 @@ static void handleObjCBridgeMutableAttr(
>                             Attr.getAttributeSpellingListIndex()));
> }
>
> +static void handleObjCDesignatedInitializer(Sema &S, Decl *D,
> +                                            const AttributeList &Attr) {
> +  SourceLocation Loc = Attr.getLoc();
> +  ObjCMethodDecl *Method = cast<ObjCMethodDecl>(D);
> +
> +  if (Method->getMethodFamily() != OMF_init) {
> +    S.Diag(D->getLocStart(),
> diag::err_attr_objc_designated_not_init_family)
> +    << SourceRange(Loc, Loc);
> +    return;
> +  }
> +  DeclContext *DC = Method->getDeclContext();
> +  if (!isa<ObjCInterfaceDecl>(DC)) {
> +    S.Diag(D->getLocStart(), diag::err_attr_objc_designated_not_interface)
> +    << SourceRange(Loc, Loc);
> +    return;
> +  }
> +
> +  Method->addAttr(::new (S.Context)
> +                  ObjCDesignatedInitializerAttr(Attr.getRange(), S.Context,
> +
> Attr.getAttributeSpellingListIndex()));
>
>
> Once you use the SubsetSubjects, this entire method can go away.
>
>
> I later added
>
>   IFace->setHasDesignatedInitializers();
>
> to this method, not sure if it can still go away now.

You're right, the method would have to stick around, but it would be
able to unilaterally call the setter and apply the attribute (and
dispense with the checks).

~Aaron