On Fri, Jan 10, 2014 at 6:56 PM, Nick Lewycky <[email protected]> wrote: > On 10 January 2014 13:42, Aaron Ballman <[email protected]> wrote: >> >> On Fri, Jan 10, 2014 at 3:43 PM, Nick Lewycky <[email protected]> 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 _______________________________________________ cfe-commits mailing list [email protected] http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
