[cfe-commits] r59148 - in /cfe/trunk: Driver/RewriteObjC.cpp include/clang/AST/Expr.h include/clang/AST/TypeOrdering.h include/clang/Basic/DiagnosticKinds.def lib/AST/Expr.cpp lib/AST/StmtSerialization.cpp lib/Sema/Sema.cpp lib/Sema/Sema.h lib/Sema/SemaDecl.cpp lib/Sema/SemaDeclCXX.cpp lib/Sema/SemaExpr.cpp lib/Sema/SemaOverload.cpp lib/Sema/SemaOverload.h test/SemaCXX/overloaded-builtin-operators.cpp

[Chris Lattner]

On Nov 12, 2008, at 9:17 AM, Douglas Gregor wrote:
> URL: http://llvm.org/viewvc/llvm-project?rev=59148&view=rev
> Log:
> Implement support for operator overloading using candidate operator
> functions for built-in operators, e.g., the builtin

Nice,

> As part of this change, ImplicitCastExpr can now be an lvalue.
> +++ cfe/trunk/include/clang/AST/Expr.h Wed Nov 12 11:17:38 2008
> @@ -834,14 +834,23 @@
> /// (*f)(), float->double, short->int, etc.
> ///
> class ImplicitCastExpr : public CastExpr {
> +  /// LvalueCast - Whether this cast produces an lvalue.
> +  bool LvalueCast;
> +
> public:

What cases can implicit casts be lvalues?  Please improve the class  
comment above to explain what invariants hold and what cases this can  
occur with.

> = 
> = 
> = 
> = 
> = 
> = 
> = 
> = 
> ======================================================================
> --- cfe/trunk/include/clang/AST/TypeOrdering.h (original)
> @@ -17,6 +17,7 @@
> #define LLVM_CLANG_TYPE_ORDERING_H
>
> #include "clang/AST/Type.h"
> +#include "llvm/ADT/DenseMap.h"

You can just use forward declaration:

template<typename T>
struct DenseMapInfo;

> +    static bool isPod() {
> +      // QualType isn't *technically* a POD type. However, we can get
> +      // away with calling it a POD type since its copy constructor,
> +      // copy assignment operator, and destructor are all trivial.
> +      return true;
> +    }

Yeah, interesting point.  This method should really be named  
'actsLikePodType' or something :)

> +++ cfe/trunk/lib/AST/Expr.cpp Wed Nov 12 11:17:38 2008
> @@ -389,8 +389,28 @@
>         cast<UnaryOperator>(this)->getOpcode() ==  
> UnaryOperator::Extension)
>       return cast<UnaryOperator>(this)->getSubExpr()- 
> >isLvalue(Ctx);  // GNU.
>     break;
> +  case ImplicitCastExprClass:
> +    return cast<ImplicitCastExpr>(this)->isLvalueCast()? LV_Valid
> +                                                       :  
> LV_InvalidExpression;
>   case ParenExprClass: // C99 6.5.1p5
>     return cast<ParenExpr>(this)->getSubExpr()->isLvalue(Ctx);
> +  case BinaryOperatorClass:
> +  case CompoundAssignOperatorClass: {
> +    const BinaryOperator *BinOp = cast<BinaryOperator>(this);
> +    if (BinOp->isAssignmentOp()) {
> +      if (Ctx.getLangOptions().CPlusPlus)
> +        // C++ [expr.ass]p1:
> +        //   The result of an assignment operation [...] is an  
> lvalue.
> +        return LV_Valid;
> +      else
> +        // C99 6.5.16:
> +        //   An assignment expression [...] is not an lvalue.
> +        return LV_InvalidExpression;
> +    } else
> +      return LV_InvalidExpression;
> +

Would it be reasonable to restructure this to reduce nesting, e.g.:

if (!BinOp->isAssignmentOp())
   return LV_InvalidExpression;

if (Ctx.getLangOptions().CPlusPlus)
   return LV_Valid;

return LV_InvalidExpression;

?  This makes it more obvious that the break it dead too.

> +QualType Sema::UsualArithmeticConversionsType(QualType lhs,  
> QualType rhs) {
> +  // Perform the usual unary conversions. We do this early so that
> +  // integral promotions to "int" can allow us to exit early, in the
> +  // lhs == rhs check. Also, for conversion purposes, we ignore any
> +  // qualifiers.  For example, "const float" and "float" are
> +  // equivalent.
> +  if (lhs->isPromotableIntegerType())
> +    lhs = Context.IntTy;
> +  else
> +    lhs = Context.getCanonicalType(lhs).getUnqualifiedType();
> +
> +  if (rhs->isPromotableIntegerType())
> +    rhs = Context.IntTy;
> +  else
> +    rhs = Context.getCanonicalType(rhs).getUnqualifiedType();

Why strip off typedefs here?  It would be much better to preserve  
them, because AFAIK, lhs/rhs end up in the AST.

> +/// BuiltinCandidateTypeSet - A set of types that will be used for  
> the
> +/// candidate operator functions for built-in operators (C++
> +/// [over.built]). The types are separated into pointer types and
> +/// enumeration types.
> +class BuiltinCandidateTypeSet  {
> +  /// TypeSet - A set of types.
> +  typedef llvm::DenseSet<QualType> TypeSet;

Is this usually small?  DenseSet is good for very large sets, but it  
does a heap allocation on the first insertion.  You might want to use  
SmallSet<QualType, 8> or something, which keeps avoids an allocation  
for sizes up to 8, but falls back to std::set after that.  Iteration  
over a SmallSet is also much faster than iteration over an extremely  
sparse DenseSet.

Another option is SmallPtr<something*, 8> (which is specialized to use  
SmallPtrSet) which falls back to an equivalent to DenseSet (but  
actually slightly better) when the set grows beyond 8 elements.  If  
you like this approach, you could either forcibly cast QualType into a  
pointer, or make a nicer specialization of SmallSet that uses  
SmallPtrSet or something.

-Chris

-Chris