[cfe-commits] r46572 - in /cfe/trunk: AST/Expr.cpp AST/StmtPrinter.cpp CodeGen/CGExprComplex.cpp CodeGen/CGExprScalar.cpp Sema/SemaExpr.cpp include/clang/AST/Expr.h include/clang/Basic/DiagnosticKinds.def

[Chris Lattner]

On Jan 30, 2008, at 12:50 PM, Nate Begeman wrote:
> URL: http://llvm.org/viewvc/llvm-project?rev=46572&view=rev
> Log:
> Implement first round of feedback on __builtin_overload

Nice!

> =
> =
> =
> =
> =
> =
> =
> =
> ======================================================================
> --- cfe/trunk/include/clang/AST/Expr.h (original)
> +++ cfe/trunk/include/clang/AST/Expr.h Wed Jan 30 14:50:20 2008
> @@ -1128,21 +1128,45 @@
> };
>
> /// OverloadExpr - Clang builtin-in function __builtin_overload.
> +/// This AST node provides a way to overload functions in C.
> +///
> +/// The first argument is required to be a constant expression, for  
> the number
> +/// of arguments passed to each candidate function.
> +///
> +/// The next N arguments, where N is the value of the constant  
> expression,
> +/// are the values to be passed as arguments.
> +///
> +/// The rest of the arguments are values of pointer to function  
> type, which
> +/// are the candidate functions for overloading.
> +///
> +/// The result is a equivalent to a CallExpr taking N arguments to  
> the
> +/// candidate function whose parameter types match the types of the  
> N arguments.
> +///
> +/// example: float Z = __builtin_overload(2, X, Y, modf, mod, modl);
> +/// If X and Y are long doubles, Z will assigned the result of  
> modl(X, Y);
> +/// If X and Y are floats, Z will be assigned the result of modf(X,  
> Y);

Very nice!

>
> class OverloadExpr : public Expr {
> +  // SubExprs - the list of values passed to the __builtin_overload  
> function.
> +  // SubExpr[0] is a constant expression
> +  // SubExpr[1-N] are the parameters to pass to the matching  
> function call
> +  // SubExpr[N-...] are the candidate functions, of type pointer to  
> function.
>   Expr **SubExprs;
> -  unsigned NumArgs;
> +
> +  // NumExprs - the size of the SubExprs array
> +  unsigned NumExprs;
> +
> +  // NumArgs - the number of arguments
> +

I think the NumArgs comment is dead?

>
> +  // The index of the matching candidate function
>   unsigned FnIndex;
> +
>   SourceLocation BuiltinLoc;
>   SourceLocation RParenLoc;
> public:
>   OverloadExpr(Expr **args, unsigned narg, unsigned idx, QualType t,

Please rename narg -> nexprs for consistency.

>
>                SourceLocation bloc, SourceLocation rploc)
> +    : Expr(OverloadExprClass, t), NumExprs(narg), FnIndex(idx),
> +      BuiltinLoc(bloc), RParenLoc(rploc) {
>     SubExprs = new Expr*[narg];
>     for (unsigned i = 0; i != narg; ++i)
>       SubExprs[i] = args[i];
> @@ -1151,19 +1175,31 @@
>     delete [] SubExprs;
>   }
>
> +  /// arg_begin - Return a pointer to the list of arguments that  
> will be passed
> +  /// to the matching candidate function, skipping over the initial  
> constant
> +  /// expression.
>   typedef Expr * const *arg_const_iterator;
>   arg_const_iterator arg_begin() const { return SubExprs+1; }

Nice.  Much more clear.

> +  /// getNumArgs - Return the number of arguments to pass to the  
> candidate
> +  /// functions.
> +  unsigned getNumArgs(ASTContext &Ctx) const {
> +    llvm::APSInt constEval(32);
> +    (void) SubExprs[0]->isIntegerConstantExpr(constEval, Ctx);
> +    return constEval.getZExtValue();
> +  }

Lazy is good.

>
> +
> +  /// getNumSubExprs - Return the size of the SubExprs array
> +  unsigned getNumSubExprs() const { return NumExprs; }

Sentence needs to end with a period.  How about also adding "this  
includes the constant expression, the actual arguments passed in, and  
the function pointers."

>
>
>   /// getArg - Return the specified argument.
> +  Expr *getExpr(unsigned Expr) {

Comment needs updating.

>
> +    assert((Expr < NumExprs) && "Arg access out of range!");
> +    return SubExprs[Expr];
>   }
> -  Expr *getFn() { return SubExprs[FnIndex]; }
> +
> +  /// getFn - Return the matching candidate function for this  
> OverloadExpr
> +  Expr *getFn() const { return SubExprs[FnIndex]; }

Sentence needs to end with a period. :)

> = 
> = 
> = 
> = 
> = 
> = 
> = 
> = 
> ======================================================================
> --- cfe/trunk/Sema/SemaExpr.cpp (original)
> +++ cfe/trunk/Sema/SemaExpr.cpp Wed Jan 30 14:50:20 2008
> @@ -2086,12 +2086,15 @@
>   return new ChooseExpr(BuiltinLoc, CondExpr, LHSExpr, RHSExpr,  
> resType, RPLoc);
> }
>
>     // Scan the parameter list for the FunctionType, checking the  
> QualType of
> -    // each paramter against the QualTypes of the arguments to the  
> builtin.
> +    // each parameter against the QualTypes of the arguments to the  
> builtin.
>     // If they match, return a new OverloadExpr.
> -    if (ExprsCompatibleWithFnType(Args+1, FnType))
> +    if (ExprsMatchFnType(Args+1, FnType))
>       return new OverloadExpr(Args, NumArgs, i, FnType- 
> >getResultType(),
>                               BuiltinLoc, RParenLoc);
>   }

This is great, but it stops checking the functions as soon as it gets  
a match.  For example, it would allow craziness like:

__builtin_overload(1, (double)x, sin, 42)

I think it should scan the whole list and then create the overload  
expr outside the loop.  This would also let you emit an error if  
multiple things match like:

__builtin_overload(1, (double)x, sin, sin)

Thanks Nate!

-Chris