[cfe-commits] r61746 - in /cfe/trunk: include/clang/AST/ include/clang/Basic/ include/clang/Parse/ lib/AST/ lib/Parse/ lib/Sema/ test/SemaCXX/

[Chris Lattner]

On Jan 5, 2009, at 12:52 PM, Sebastian Redl wrote:
> URL: http://llvm.org/viewvc/llvm-project?rev=61746&view=rev
> Log:
> PODness and Type Traits
>
> Make C++ classes track the POD property (C++ [class]p4)
> Track the existence of a copy assignment operator.
> Implicitly declare the copy assignment operator if none is provided.
> Implement most of the parsing job for the G++ type traits extension.
> Fully implement the low-hanging fruit of the type traits:
> __is_pod: Whether a type is a POD.
> __is_class: Whether a type is a (non-union) class.
> __is_union: Whether a type is a union.
> __is_enum: Whether a type is an enum.
> __is_polymorphic: Whether a type is polymorphic (C++  
> [class.virtual]p1).

Very nice!

> +bool CXXRecordDecl::hasConstCopyAssignment(ASTContext &Context)  
> const {
..
> +    // Don't assert on this; an invalid decl might have been left  
> in the AST.
> +    if (FnType->getNumArgs() != 1 || FnType->isVariadic())
> +      continue;

Generally, invalid ASTs aren't created: they are either dropped or  
munged into something valid.  Can you give an example that would  
trigger this?

>
> +void CXXRecordDecl::addedAssignmentOperator(ASTContext &Context,
> +                                            CXXMethodDecl *OpDecl) {
> +  // We're interested specifically in copy assignment operators.
> +  // Unlike addedConstructor, this method is not called for implicit
> +  // declarations.
> +  const FunctionTypeProto *FnType = OpDecl->getType()- 
> >getAsFunctionTypeProto();
> +  assert(FnType && "Overloaded operator has no proto function  
> type.");
> +  assert(FnType->getNumArgs() == 1 && !FnType->isVariadic());

> +  QualType ArgType = FnType->getArgType(0);
> +  if (const ReferenceType *Ref = ArgType->getAsReferenceType())
> +    ArgType = Ref->getPointeeType();

How about:

QualType ArgType = FnType->getArgType(0).getNonReferenceType();

?

>
> +
> +  ArgType = ArgType.getUnqualifiedType();
> +  QualType ClassType =  
> Context.getCanonicalType(Context.getTypeDeclType(
> +    const_cast<CXXRecordDecl*>(this)));
> +
> +  if (ClassType != Context.getCanonicalType(ArgType))
> +    return;

Doing this comparison with types seems awkward, can you just do  
something like:

if (const RecordType *RT = ArgType->getAsRecordType())
   if (RT->getDecl() == this) ...

instead?  likewise in hasConstCopyAssignment.

> = 
> = 
> = 
> = 
> = 
> = 
> = 
> = 
> ======================================================================
> --- cfe/trunk/lib/AST/Expr.cpp (original)
> +++ cfe/trunk/lib/AST/Expr.cpp Mon Jan  5 14:52:13 2009
> @@ -1092,6 +1092,10 @@
>   case CXXDefaultArgExprClass:
>     return cast<CXXDefaultArgExpr>(this)
>              ->isIntegerConstantExpr(Result, Ctx, Loc, isEvaluated);
> +
> +  case UnaryTypeTraitExprClass:
> +    Result = cast<UnaryTypeTraitExpr>(this)->Evaluate();
> +    return true;
>   }

Please mention these at the end of http://clang.llvm.org/docs/InternalsManual.html#Constants 
.

> +/// ParseUnaryTypeTrait - Parse the built-in unary type-trait
> +/// pseudo-functions that allow implementation of the TR1/C++0x  
> type traits
> +/// templates.
> +///
> +///       primary-expression:
> +/// [GNU]             unary-type-trait '(' type-id ')'
> +///
> +Parser::OwningExprResult Parser::ParseUnaryTypeTrait()
> +{
> +  UnaryTypeTrait UTT = UnaryTypeTraitFromTokKind(Tok.getKind());
> +  SourceLocation Loc = ConsumeToken();
> +
> +  SourceLocation LParen = Tok.getLocation();
> +  if (ExpectAndConsume(tok::l_paren, diag::err_expected_lparen))
> +    return ExprError();
> +
> +  // FIXME: Error reporting absolutely sucks! If the this fails to  
> parse a type
> +  // there will be cryptic errors about mismatched parentheses and  
> missing
> +  // specifiers.
> +  TypeTy *Ty = ParseTypeName();

Wow, you're right.  ParseTypeName should really return a pair of  
success+type instead of just type.

> +
> +  SourceLocation RParen = MatchRHSPunctuation(tok::r_paren, LParen);
> +
> +  return Actions.ActOnUnaryTypeTrait(UTT, Loc, LParen, Ty, RParen);
> +}

Likewise, is there is an error in MatchRHSPunctuation, the  
ActOnUnaryTypeTrait action really shouldn't be called.



> +  if (getLangOptions().CPlusPlus) {
>     CheckExtraCXXDefaultArguments(D);
> +    if (!T->isPODType())
> +      cast<CXXRecordDecl>(Record)->setPOD(false);

I see that you basically clear the 'ispod' bit whenever something is  
parsed that prevents a class from being a POD.  Would it be better to  
make it a predicate that walks the member list to determine podness?   
The advantage of doing this is that it would put all the "podness"  
checks together into one method instead of scattered throughout sema.   
What do you think?

> +
> +    //   An implicitly-declared copy assignment operator is an  
> inline public
> +    //   member of its class.
> +    DeclarationName Name =
> +      Context.DeclarationNames.getCXXOperatorName(OO_Equal);
> +    CXXMethodDecl *CopyAssignment =
> +      CXXMethodDecl::Create(Context, ClassDecl, ClassDecl- 
> >getLocation(), Name,
> +                            Context.getFunctionType(RetType,  
> &ArgType, 1,
> +                                                    false, 0),
> +                            /*isStatic=*/false, /*isInline=*/true,  
> 0);
> +    CopyAssignment->setAccess(AS_public);

My reading of this code is that it creates the decl eagerly when the  
class is defined if it doesn't already have one.  Does this mean that  
simple things like:

struct foo { int x, y; };

will get these decls?  Would it be possible to synthesize these lazily  
like G++ does?

Nice work!

-Chris