[cfe-commits] r46132 - in /cfe/trunk: AST/Expr.cpp AST/StmtPrinter.cpp CodeGen/CGBuiltin.cpp CodeGen/CGExpr.cpp CodeGen/CGExprScalar.cpp CodeGen/CodeGenFunction.h Parse/ParseExpr.cpp Sema/Sema.h Sema/SemaExpr.cpp include/clang/AST/Expr.h include/clang/AST/StmtNodes.def include/clang/Basic/DiagnosticKinds.def include/clang/Basic/TokenKinds.def include/clang/Parse/Action.h

[Chris Lattner]

On Jan 17, 2008, at 9:46 AM, Nate Begeman wrote:
> URL: http://llvm.org/viewvc/llvm-project?rev=46132&view=rev
> Log:
> Implement basic overload support via a new builtin,  
> __builtin_overload.

Hey Nate,

I'm finally starting to catch up on clang-mail and you're at the top  
of the list.  :)

> __builtin_overload takes 2 or more arguments:
> 0) a non-zero constant-expr for the number of arguments the overloaded
>   functions will take
> 1) the arguments to pass to the matching overloaded function
> 2) a list of functions to match.
>
> The return type of __builtin_overload is inferred from the function  
> whose args
> match the types of the arguments passed to the builtin.  For example:
>
> float a;
> float sinf(float);
> int   sini(int);
>
> float b = __builtin_overload(1, a, sini, sinf);
>
> Says that we are overloading functions that take one argument, and  
> trying to
> pass an argument of the same type as 'a'.  sini() does not match  
> since it takes
> and argument of type int.  sinf does match, so at codegen time this  
> will turn
> into float b = sinf(a);

Ok, nice!  Please add a couple tests to the testsuite though.  It is  
very useful to have testcases and documentation for things that are  
clang extensions.  Some more nit picks:

> = 
> = 
> = 
> = 
> = 
> = 
> = 
> = 
> ======================================================================
> --- cfe/trunk/include/clang/AST/Expr.h (original)
> +++ cfe/trunk/include/clang/AST/Expr.h Thu Jan 17 11:46:27 2008
> @@ -1124,6 +1124,57 @@
>   virtual child_iterator child_end();
> };
>
> +/// OverloadExpr - Clang builtin-in function __builtin_overload.
> +/// This AST node provides a way to overload functions in C
> +/// i.e. float Z = __builtin_overload(2, X, Y, modf, mod, modl);
> +/// would pick whichever of the functions modf, mod, and modl that  
> took two
> +/// arguments of the same type as X and Y.

It would be good to mention here that the function arguments are  
required to have pointer to function type.

> +class OverloadExpr : public Expr {
> +  Expr **SubExprs;

Please describe what the subexprs are here.   Is SubExprs[0] the  
constant expression?

>
> +  unsigned NumArgs;
> +  unsigned FnIndex;

Can FnIndex be computed lazily from SubExprs[0] instead of being  
stored in the AST?

> +  typedef Expr * const *arg_const_iterator;
> +  arg_const_iterator arg_begin() const { return SubExprs+1; }

Why does arg_begin skip the first element here?  What is element 0?

> +  /// getNumArgs - Return the number of actual arguments to this  
> call.
> +  ///
> +  unsigned getNumArgs() const { return NumArgs; }

 From context, I don't think this is right.  Does this include the  
constant expression and the arguments and the functions?  Or just a  
subset of these?  Please clarify in the comment.

> = 
> = 
> = 
> = 
> = 
> = 
> = 
> = 
> ======================================================================
> --- cfe/trunk/CodeGen/CGExprScalar.cpp (original)
> +++ cfe/trunk/CodeGen/CGExprScalar.cpp Thu Jan 17 11:46:27 2008
> @@ -991,6 +992,10 @@
>     Visit(E->isConditionTrue(CGF.getContext()) ? E->getLHS() : E- 
> >getRHS());
> }
>
> +Value *ScalarExprEmitter::VisitOverloadExpr(OverloadExpr *E) {
> +  return CGF.EmitCallExpr(E->getFn(), E->arg_begin()).getScalarVal();
> +}

I think you need similar functionality it CGExpr(Complex|Agg) to  
handle complex and aggregate returns.

> = 
> = 
> = 
> = 
> = 
> = 
> = 
> = 
> ======================================================================
> --- cfe/trunk/Parse/ParseExpr.cpp (original)
> +++ cfe/trunk/Parse/ParseExpr.cpp Thu Jan 17 11:46:27 2008
> @@ -909,6 +911,44 @@
>                                   ConsumeParen());
>     break;
>   }
> +  case tok::kw___builtin_overload: {
> +    llvm::SmallVector<ExprTy*, 8> ArgExprs;
> +    llvm::SmallVector<SourceLocation, 8> CommaLocs;
> +
...

> +    // __builtin_overload requires at least 2 arguments
> +    if (ArgExprs.size() < 2) {
> +      Diag(Tok, diag::err_typecheck_call_too_few_args);
> +      return ExprResult(true);
> +    }

I think it would make sense to do this in Sema.  That all the  
correctness checking is in one place, making it more obvious that the  
access to Args[0] is safe.

> = 
> = 
> = 
> = 
> = 
> = 
> = 
> = 
> ======================================================================
> --- cfe/trunk/Sema/SemaExpr.cpp (original)
> +++ cfe/trunk/Sema/SemaExpr.cpp Thu Jan 17 11:46:27 2008

> +/// ExprsCompatibleWithFnType - return true if the Exprs in array  
> Args have
> +/// QualTypes that match the QualTypes of the arguments of the  
> FnType.

Mention in this comment that we already validated that the #args  
matches the number of args in FnType.  Also, 'compatible' is a highly  
loaded word in the standard that doesn't mean 'exact match'.  How  
about calling it "ExprsMatchFnType"?
>
> +static bool ExprsCompatibleWithFnType(Expr **Args,  
> FunctionTypeProto *FnType) {
can the arguments be 'const'?
>
> +  unsigned NumParams = FnType->getNumArgs();
> +  for (unsigned i = 0; i != NumParams; ++i)
> +    if (Args[i]->getType() != FnType->getArgType(i))

This needs to use getType().getCanonicalType() to avoid problems with  
typedef etc.

> +
> +Sema::ExprResult Sema::ActOnOverloadExpr(ExprTy **args, unsigned  
> NumArgs,
> +                                         SourceLocation *CommaLocs,
> +                                         SourceLocation BuiltinLoc,
> +                                         SourceLocation RParenLoc) {
> +  assert((NumArgs > 1) && "Too few arguments for OverloadExpr!");

> +  // Figure out the return type, by matching the args to one of the  
> functions
> +  // listed after the paramters.

typo 'paramters'

> +  for (unsigned i = NumParams + 1; i < NumArgs; ++i) {
> +    // UsualUnaryConversions will convert the function DeclRefExpr  
> into a
> +    // pointer to function.
> +    Expr *Fn = UsualUnaryConversions(Args[i]);
> +    FunctionTypeProto *FnType = 0;
> +    if (const PointerType *PT = Fn->getType()->getAsPointerType())
> +      FnType = dyn_cast<FunctionTypeProto>(PT->getPointeeType());

This won't work 100% in the face of typedefs.  Please dyncast "PT- 
 >getPointeeType().getCanonicalType()", which will strip off the  
typedef'ness.

> +    // The Expr type must be FunctionTypeProto, since  
> FunctionTypeProto has no
> +    // parameters, and the number of parameters must match the  
> value passed to
> +    // the builtin.
> +    if (!FnType || (FnType->getNumArgs() != NumParams))
> +      continue;

I think it makes sense to emit an error about these two cases.  It  
doesn't ever make sense to use:

   __builtin_overload(1, x, fmod)
of:
   __builtin_overload(1, x, 42)

for example, because fmod takes two parameters, and the int isn't a  
function parameter.  This should be a hard error instead of silently  
ignoring it.

>
> +
> +    // Scan the parameter list for the FunctionType, checking the  
> QualType of
> +    // each paramter against the QualTypes of the arguments to the  
> builtin.

typo paramter

> +    // If they match, return a new OverloadExpr.
> +    if (ExprsCompatibleWithFnType(Args+1, FnType))
> +      return new OverloadExpr(Args, NumArgs, i, FnType- 
> >getResultType(),
> +                              BuiltinLoc, RParenLoc);
> +  }
> +
> +  // If we didn't find a matching function Expr in the  
> __builtin_overload list
> +  // the return an error.
> +  std::string typeNames;
> +  for (unsigned i = 0; i != NumParams; ++i)
> +    typeNames += Args[i+1]->getType().getAsString() + " ";

This adds an extraneous ' ' at the end of string :)

Overall, very nice Nate!

-Chris