patch: new attribute enable_if
Aaron Ballman
aaron at aaronballman.com
Fri Jan 10 16:14:34 PST 2014
On Fri, Jan 10, 2014 at 6:56 PM, Nick Lewycky <nlewycky at google.com> wrote:
> On 10 January 2014 13:42, Aaron Ballman <aaron at aaronballman.com> wrote:
>>
>> On Fri, Jan 10, 2014 at 3:43 PM, Nick Lewycky <nicholas at mxc.ca> wrote:
>> > Aaron Ballman wrote:
>> >>
>> >> This is fantastic, thank you for taking this on! Some minor comments
>> >> and observations below.
>> >
>> >
>> > Thank you for the thorough review! I haven't been able to fix everything
>> > you
>> > pointed out, but I'm attaching an updated patch anyways, with FIXME's in
>> > it.
>> >
>> > The two issues unfixed are:
>> > - Why does C++11 syntax think there's only one argument?
>> > - Why is potential-constexpr evaluation failing on VarTemplateDecls?
>> >
>> >>> Index: docs/LanguageExtensions.rst
>> >>> ===================================================================
>> >>> --- docs/LanguageExtensions.rst (revision 198923)
>> >>> +++ docs/LanguageExtensions.rst (working copy)
>> >>> @@ -1316,6 +1316,8 @@
>> >>> Query the presence of this new mangling with
>> >>> ``__has_feature(objc_protocol_qualifier_mangling)``.
>> >>>
>> >>> +.. _langext-overloading:
>> >>> +
>> >>> Function Overloading in C
>> >>> =========================
>> >>>
>> >>> @@ -1398,6 +1400,84 @@
>> >>>
>> >>> Query for this feature with
>> >>> ``__has_extension(attribute_overloadable)``.
>> >>>
>> >>> +Controlling Overload Resolution
>> >>> +===============================
>> >>> +
>> >>> +Clang introduces the ``enable_if`` attribute, which can be placed on
>> >>> function
>> >>> +declarations to control which overload is selected based on the
>> >>> values
>> >>> of the
>> >>> +function's arguments. When combined with the
>> >>> +:ref:``overloadable<langext-overloading>`` attribute, this feature is
>> >>> also
>> >>> +available in C.
>> >>> +
>> >>> +.. code-block:: c++
>> >>> +
>> >>> + int isdigit(int c);
>> >>> + int isdigit(int c) __attribute__((enable_if(c>= -1&& c<= 255, "'c'
>> >>> must have the value of an unsigned char or EOF")));
>> >>>
>> >>> +
>> >>> + void foo(char c) {
>> >>> + isdigit(c);
>> >>> + isdigit(10);
>> >>> + isdigit(-10); // results in a compile-time error.
>> >>> + }
>> >>> +
>> >>> +The enable_if attribute takes two arguments, the first is an
>> >>> expression
>> >>> written
>> >>> +in terms of the function parameters, the second is a string
>> >>> explaining
>> >>> why this
>> >>> +overload candidate could not be selected to be displayed in
>> >>> diagnostics.
>> >>> The
>> >>> +expression is part of the function signature for the purposes of
>> >>> determining
>> >>> +whether it is a redeclaration (following the rules used when
>> >>> determining
>> >>> +whether a C++ template specialization is ODR-equivalent), but is not
>> >>> part of
>> >>> +the type.
>> >>> +
>> >>> +An enable_if expression will be evaluated by substituting the values
>> >>> of
>> >>> the
>> >>> +parameters from the call site into the arguments in the expression
>> >>> and
>> >>> +determining whether the result is true. If the result is false or
>> >>> could
>> >>> not be
>> >>> +determined through constant expression evaluation, then this overload
>> >>> will not
>> >>> +be chosen and the reason supplied in the string will be given to the
>> >>> user if
>> >>> +their code does not compile as a result.
>> >>> +
>> >>> +Because the enable_if expression is an unevaluated context, there are
>> >>> no
>> >>> global
>> >>> +state changes, nor the ability to pass information from the enable_if
>> >>> +expression to the function body. As a worked example, suppose we want
>> >>> calls to
>> >>
>> >>
>> >> "As a worked example" reads a bit funny to me, is it a typo?
>> >
>> >
>> > Hm, it's a phrase I use to mean "an example that's been worked out to
>> > show
>> > the reader how to do it" but that doesn't appear to be common usage.
>> > Fixed
>> > to simply use "For example".
>> >
>> >
>> >>> +strnlen(strbuf, maxlen) to resolve to strnlen_chk(strbuf, maxlen,
>> >>> size
>> >>> of
>> >>> +strbuf) only if the size of strbuf can be determined:
>> >>> +
>> >>> +.. code-block:: c++
>> >>> +
>> >>> + __attribute__((always_inline))
>> >>> + static inline size_t strnlen(const char *s, size_t maxlen)
>> >>> + __attribute__((overloadable))
>> >>> + __attribute__((enable_if(__builtin_object_size(s, 0) != -1))),
>> >>> + "chosen when the buffer size is known
>> >>> but
>> >>> 'maxlen' is not")))
>> >>> + {
>> >>> + return strnlen_chk(s, maxlen, __builtin_object_size(s, 0));
>> >>> + }
>> >>> +
>> >>> +Multiple enable_if attributes may be applied to a single declaration.
>> >>> In
>> >>> this
>> >>> +case, the enable_if expressions are evaluated from left to right in
>> >>> the
>> >>> +following manner. First the candidates whose enable_if expressions
>> >>> evaluate to
>> >>
>> >>
>> >> "First" should be followed by a comma. Wow, that's nitpicky. ;-)
>> >
>> >
>> > Done. :)
>> >
>> >>> +false or cannot be evaluated are discarded. If the remaining
>> >>> candidates
>> >>> do not
>> >>> +share ODR-equivalent enable_if expressions, the overload resolution
>> >>> is
>> >>> +ambiguous. Otherwise, enable_if overload resolution continues with
>> >>> the
>> >>> next
>> >>> +enable_if attribute on the candidates that have not been discarded
>> >>> and
>> >>> have
>> >>> +remaining enable_if attributes. In this way, we pick the most
>> >>> specific
>> >>> +overload out of a number of viable overloads using enable_if.
>> >>> +
>> >>> +.. code-block:: c++
>> >>> + void f() __attribute__((enable_if(true, ""))); // #1
>> >>> + void f() __attribute__((enable_if(true, "")))
>> >>> __attribute__((enable_if(true, ""))); // #2
>> >>> +
>> >>> + void g(int i, int j) __attribute__((enable_if(i, ""))); // #1
>> >>> + void g(int i, int j) __attribute__((enable_if(j, "")))
>> >>> __attribute__((enable_if(true))); // #2
>> >>> +
>> >>> +In this example, a call to f() is always resolved to #2, as the first
>> >>> enable_if
>> >>> +expression is ODR-equivalent for both declarations, but #1 does not
>> >>> have
>> >>> another
>> >>> +enable_if expression to continue evaluating, so the next round of
>> >>> evaluation has
>> >>> +only a single candidate. In a call to g(1, 1), the call is ambiguous
>> >>> even though
>> >>> +#2 has more enable_if attributes, because the first enable_if
>> >>> expressions are
>> >>> +not ODR-equivalent.
>> >>> +
>> >>> +Query for this feature with ``__has_attribute(enable_if)``.
>> >>> +
>> >>> Initializer lists for complex numbers in C
>> >>> ==========================================
>> >>>
>> >>> Index: include/clang/AST/Expr.h
>> >>> ===================================================================
>> >>> --- include/clang/AST/Expr.h (revision 198923)
>> >>> +++ include/clang/AST/Expr.h (working copy)
>> >>> @@ -508,6 +508,16 @@
>> >>> SmallVectorImpl<
>> >>> PartialDiagnosticAt>
>> >>> &Diags);
>> >>>
>> >>> + /// isPotentialConstantExprUnevaluted - Return true if this
>> >>> expression
>> >>> might
>> >>> + /// be usable in a constant expression in C++11 in an unevaluated
>> >>> context, if
>> >>> + /// it were in function FD marked constexpr. Return false if the
>> >>> function can
>> >>> + /// never produce a constant expression, along with diagnostics
>> >>> describing
>> >>> + /// why not.
>> >>> + static bool isPotentialConstantExprUnevaluated(Expr *E,
>> >>> + const FunctionDecl
>> >>> *FD,
>> >>> + SmallVectorImpl<
>> >>> +
>> >>> PartialDiagnosticAt>
>> >>> &Diags);
>> >>> +
>> >>> /// isConstantInitializer - Returns true if this expression can be
>> >>> emitted to
>> >>> /// IR as a constant, and thus can be used as a constant
>> >>> initializer
>> >>> in C.
>> >>> bool isConstantInitializer(ASTContext&Ctx, bool ForRef) const;
>> >>>
>> >>> @@ -600,6 +610,14 @@
>> >>> const VarDecl *VD,
>> >>> SmallVectorImpl<PartialDiagnosticAt>
>> >>> &Notes) const;
>> >>>
>> >>> + /// EvaluateWithSubstitution - Evaluate an expression as if from
>> >>> the
>> >>> context
>> >>> + /// of a call to the given function with the given arguments,
>> >>> inside
>> >>> an
>> >>> + /// unevaluated context. Returns true if the expression could be
>> >>> folded to a
>> >>> + /// constant.
>> >>> + bool EvaluateWithSubstitution(APValue&Value, ASTContext&Ctx,
>> >>> + FunctionDecl *Callee,
>> >>> + llvm::ArrayRef<const Expr*> Args)
>> >>> const;
>> >>> +
>> >>> /// \brief Enumeration used to describe the kind of Null pointer
>> >>> constant
>> >>> /// returned from \c isNullPointerConstant().
>> >>> enum NullPointerConstantKind {
>> >>> Index: include/clang/Basic/Attr.td
>> >>> ===================================================================
>> >>> --- include/clang/Basic/Attr.td (revision 198923)
>> >>> +++ include/clang/Basic/Attr.td (working copy)
>> >>> @@ -470,6 +470,13 @@
>> >>> let Subjects = SubjectList<[Function]>;
>> >>> }
>> >>>
>> >>> +def EnableIf : InheritableAttr {
>> >>> + let Spellings = [GNU<"enable_if">];
>> >>
>> >>
>> >> Since this is also supported in C++, could we have a CXX11<"clang",
>> >> "enable_if"> spelling as well?
>> >
>> >
>> > I tried adding that, but it doesn't seem work out of the box:
>> >
>> > void f(int n) [[clang::enable_if((n > 5), "chosen when 'n' is greater
>> > than
>> > five")]];
>> >
>> > produces "error: expected expression" while:
>> >
>> > [[clang::enable_if((n > 5), "chosen when 'n' is greater than five")]]
>> > void
>> > f(int n);
>> >
>> > produces "error: 'enable_if' attribute requires exactly 2 arguments"!
>> >
>> > That's going to take some debugging. I've added a testcase for this
>> > commented out with a FIXME on it.
>>
>> I can save you the debugging -- I totally forgot that C++11 attribute
>> parsing doesn't support arguments. I'll put that on my list of things
>> to look into sometime in the near-ish future though. Thanks for the
>> reminder! :-)
>
>
> Hah! Thanks. I'm going to pull the spelling out of Attrs.td and leave the
> FIXME in the testcase.
>
>> >>> + let Subjects = SubjectList<[Function]>;
>> >>
>> >>
>> >> Do you want this to apply to FunctionTemplate and perhaps ObjCMethod as
>> >> well?
>> >
>> >
>> > To quote Richard Smith, "this [attribute] should apply to the
>> > FunctionDecl
>> > within a FunctionTemplateDecl, not to the template itself, right?" -
>> >
>> > http://lists.cs.uiuc.edu/pipermail/cfe-commits/Week-of-Mon-20130923/089332.html
>>
>> Hmmm, I may have to educate myself on this point a bit more then.
>>
>> >
>> > I'm not sure about ObjCMethod. I'm leaning towards not supporting it
>> > simply
>> > because I don't know enough objc to write any testcases. I don't have
>> > any
>> > ideological opposition here.
>> >
>> >
>> >>> + let Args = [ExprArgument<"Cond">, StringArgument<"Message">];
>> >>> + let TemplateDependent = 1;
>> >>
>> >>
>> >> You *may* have to set LateParsed = 1 here as well, as the expressions
>> >> are template dependant. I'm not 100% certain though.
>> >
>> >
>> > Uh oh. I musn't set LateParsed = 1 here, because we need to parse it
>> > before
>> > we can decide whether it's a redeclaration. Late parsed is too late.
>> >
>> > Unfortunately, enable_if on a templated function looks pretty broken,
>> > see my
>> > comments at the end. At the moment, I don't think that LateParsed = 0 is
>> > the
>> > problem, but until I finish debugging it I can't be sure.
>> >
>> >
>> >>> +}
>> >>> +
>> >>> def ExtVectorType : Attr {
>> >>> let Spellings = [GNU<"ext_vector_type">];
>> >>> let Subjects = SubjectList<[TypedefName], ErrorDiag>;
>> >>> Index: include/clang/Basic/DiagnosticSemaKinds.td
>> >>> ===================================================================
>> >>> --- include/clang/Basic/DiagnosticSemaKinds.td (revision 198923)
>> >>> +++ include/clang/Basic/DiagnosticSemaKinds.td (working copy)
>> >>> @@ -1739,6 +1739,8 @@
>> >>> def ext_constexpr_function_never_constant_expr : ExtWarn<
>> >>> "constexpr %select{function|constructor}0 never produces a"
>> >>> "constant expression">, InGroup<DiagGroup<"invalid-constexpr">>,
>> >>> DefaultError;
>> >>> +def err_enable_if_never_constant_expr : Error<
>> >>> + "enable_if attribute expression never produces a constant
>> >>> expression">;
>> >>
>> >>
>> >> I'd prefer the diagnostic to have enable_if in single quotes (it's
>> >> more consistent with other attribute diagnostics then).
>> >
>> >
>> > Done.
>> >
>> >
>> >>> def err_constexpr_body_no_return : Error<
>> >>> "no return statement in constexpr function">;
>> >>> def warn_cxx11_compat_constexpr_body_no_return : Warning<
>> >>> @@ -2588,6 +2590,8 @@
>> >>> "candidate template ignored: substitution failure%0%1">;
>> >>> def note_ovl_candidate_disabled_by_enable_if : Note<
>> >>> "candidate template ignored: disabled by %0%1">;
>> >>> +def note_ovl_candidate_disabled_by_enable_if_attr : Note<
>> >>> + "candidate disabled: %0">;
>> >>> def note_ovl_candidate_failed_overload_resolution : Note<
>> >>> "candidate template ignored: couldn't resolve reference to
>> >>> overloaded"
>> >>> "function %0">;
>> >>> Index: include/clang/Parse/Parser.h
>> >>> ===================================================================
>> >>> --- include/clang/Parse/Parser.h (revision 198923)
>> >>> +++ include/clang/Parse/Parser.h (working copy)
>> >>> @@ -1972,7 +1972,7 @@
>> >>> if (Tok.is(tok::kw___attribute)) {
>> >>> ParsedAttributes attrs(AttrFactory);
>> >>> SourceLocation endLoc;
>> >>> - ParseGNUAttributes(attrs,&endLoc, LateAttrs);
>> >>> + ParseGNUAttributes(attrs,&endLoc, LateAttrs,&D);
>> >>> D.takeAttributes(attrs, endLoc);
>> >>> }
>> >>> }
>> >>> @@ -1984,14 +1984,16 @@
>> >>> }
>> >>> void ParseGNUAttributes(ParsedAttributes&attrs,
>> >>> SourceLocation *endLoc = 0,
>> >>> - LateParsedAttrList *LateAttrs = 0);
>> >>> + LateParsedAttrList *LateAttrs = 0,
>> >>> + Declarator *D = 0);
>> >>> void ParseGNUAttributeArgs(IdentifierInfo *AttrName,
>> >>> SourceLocation AttrNameLoc,
>> >>> ParsedAttributes&Attrs,
>> >>> SourceLocation *EndLoc,
>> >>> IdentifierInfo *ScopeName,
>> >>> SourceLocation ScopeLoc,
>> >>> - AttributeList::Syntax Syntax);
>> >>> + AttributeList::Syntax Syntax,
>> >>> + Declarator *D);
>> >>> IdentifierLoc *ParseIdentifierLoc();
>> >>>
>> >>> void MaybeParseCXX11Attributes(Declarator&D) {
>> >>> Index: include/clang/Sema/Overload.h
>> >>> ===================================================================
>> >>> --- include/clang/Sema/Overload.h (revision 198923)
>> >>> +++ include/clang/Sema/Overload.h (working copy)
>> >>> @@ -579,7 +579,11 @@
>> >>> /// (CUDA) This candidate was not viable because the callee
>> >>> /// was not accessible from the caller's target (i.e.
>> >>> host->device,
>> >>> /// global->host, device->host).
>> >>> - ovl_fail_bad_target
>> >>> + ovl_fail_bad_target,
>> >>> +
>> >>> + /// This candidate function was not viable because an enable_if
>> >>> + /// attribute disabled it.
>> >>> + ovl_fail_enable_if
>> >>> };
>> >>>
>> >>> /// OverloadCandidate - A single candidate in an overload set (C++
>> >>> 13.3).
>> >>> Index: include/clang/Sema/Sema.h
>> >>> ===================================================================
>> >>> --- include/clang/Sema/Sema.h (revision 198923)
>> >>> +++ include/clang/Sema/Sema.h (working copy)
>> >>> @@ -101,6 +101,7 @@
>> >>> class DependentDiagnostic;
>> >>> class DesignatedInitExpr;
>> >>> class Designation;
>> >>> + class EnableIfAttr;
>> >>> class EnumConstantDecl;
>> >>> class Expr;
>> >>> class ExtVectorType;
>> >>> @@ -2200,6 +2201,11 @@
>> >>> // identified by the expression Expr
>> >>> void NoteAllOverloadCandidates(Expr* E, QualType DestType =
>> >>> QualType());
>> >>>
>> >>> + /// Check the enable_if expressions on the given function. Returns
>> >>> the
>> >>> first
>> >>> + /// failing attribute, or NULL if they were all successful.
>> >>> + EnableIfAttr *CheckEnableIf(FunctionDecl *Function, ArrayRef<Expr
>> >>> *>
>> >>> Args,
>> >>> + bool MissingImplicitThis = false);
>> >>> +
>> >>
>> >>
>> >> A bit of const love here would not make me sad, but isn't strictly
>> >> required.
>> >
>> >
>> > I tried making using ArrayRef<const Expr *> here but ended up pulling a
>> > very
>> > long string (or else using bad const_casts). It's not worth it.
>>
>> I was thinking more for the returned Attrm the given FunctionDecl and
>> the CheckEnableIf function itself (if possible).
>
>
> The returned Attr* can't be (without const_casts) because it's a container
> whose member is copied into DeductionFailureInfo::Data. Making that const
> breaks other users who expect to stick a non-const Expr* in there and get
> one back out. No dice.
>
> Making the FunctionDecl* const fails when trying to use
> PerformObjectArgumentInitialization / InitializeParameter which don't accept
> const, and I'm not threading it through there.
>
> Qualifying the method itself const means that I 'this' is now const, and I
> can't change state like creating a SFINAETrap, nor can I call
> PerformObjectArgumentInitialization.
Well that certainly explains that! Thanks! :-)
~Aaron
More information about the cfe-commits
mailing list