[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

[Douglas Gregor]

On Nov 12, 2008, at 5:01 PM, Chris Lattner wrote:

> 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.

It happens when binding a value to a reference. I've fix the comment  
accordingly.

>> = 
>> =====================================================================
>> --- 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;

Okay.

>
>> +    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 :)

isStandardLayoutType, for the C++0x'ers around...

>
>> +++ 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.

Sure.

>
>> +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.

Huh, why did I do that? Fixed now.

>
>> +/// 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.

Ouch! That's good to know...

>  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.

Okay, I'll pick one of these and leave a FIXME to revisit this once we  
can parse real C++ code. I'm guessing we'll see a bimodal distribution  
in the number of types here, with most cases having no entries and a  
small number of cases having many entries.

Thanks for the review!

   - Doug