[cfe-dev] [PATCH] Function-style casting

[Chris Lattner]

On Aug 16, 2008, at 2:45 PM, Argiris Kirtzidis wrote:

> Hi,
>
> The attached patch adds support for C++'s "Type-Spec( expression- 
> list )" expression:
>
> -The Parser calls a new "ActOnCXXTypeConstructExpr" action.
> -Sema, depending on the type and expressions number:
>    -If the type is a class, treats it as a class constructor [TODO]
>    -If there's only one expression (i.e. "int(0.5)" ), creates a  
> "CXXFunctionalCastExpr" Expr node
>    -If there are no expressions (i.e "int()" ), creates a  
> "CXXZeroInitValueExpr" Expr node.
>

Wow, very nice!  Some thoughts:


+/// CXXFunctionalCastExpr - [C++ 5.2.3p1] Explicit type conversion
+///                                       (functional notation).
+///

Please add an example (e.g. "int(0.5)") to the comment.  Also,  
CXXFunctionalCastExpr has no out-of-line virtual functions, which  
means the compiler will splat vtables for it into every translation  
unit that references it.  Please give it an out-of-line virtual  
function.  There are probably other ASTs that are similarly naughty. :)


+class CXXFunctionalCastExpr : public CastExpr {

Deriving from CastExpr means that this has a location for the lparen,  
which is somewhat unneeded.  We also have a bit of strangeness because  
ImplicitCast and Cast are unrelated.  As a follow-on patch, what do  
you think of this sort of class hierarchy:

Expr
   -> CastExpr
      -> ExplicitCastExpr
      -> ImplicitCastExpr
      -> CXXFunctionalCastExpr

CastExpr would just have a subexpr but no location.  Does that seem  
reasonable?

+/// CXXZeroInitValueExpr - [C++ 5.2.3p2]
+/// Expression "T()" which creates a value-initialized Rvalue of type  
T.
+///
+class CXXZeroInitValueExpr : public Expr {
+  SourceLocation TyBeginLoc;
+  SourceLocation LParenLoc;
+  SourceLocation RParenLoc;

It might be worthwhile to point out that this is not used for  
constructors, by saying someting like "of non-class type T" or  
something if appropriate.  Please don't include the lparen loc, we  
should add it on demand if some client needs it.

  /// [OBJC]  objc-string-literal
+/// [C++]   simple-type-specifier '(' expression-list[opt] ')'      [C 
++ 5.2.3]
+/// [C++]   typename-specifier '(' expression-list[opt]  
')'         [TODO]
  /// [C++]   'const_cast' '<' type-name '>' '(' expression  
')'       [C++ 5.2p1]

The C++ Spec lists these as postfix exprs, not primary exprs.  Does it  
affect precedence or matter in practice?

+  case tok::identifier: {
+    if (getLang().CPlusPlus &&
+        Actions.isTypeName(*Tok.getIdentifierInfo(), CurScope))
+      goto HandleType;


Please add a comment describing what this is doing with a simple  
example, e.g. "Handle C++ constructor-style cast [e.g. "T(4.5)"] if T  
is a typedef for double"


+  // Match the ')'.
+  if (Tok.isNot(tok::r_paren)) {
+    MatchRHSPunctuation(tok::r_paren, LParenLoc);
+    return ExprResult(true);
+  }
+
+  SourceLocation RParenLoc = ConsumeParen();

Does this work instead?:

+  // Match the ')'.
+  SourceLocation RParenLoc = MatchRHSPunctuation(tok::r_paren,  
LParenLoc);

The intention of MatchRHSPunctuation is that you just unconditionally  
call it to simplify the caller.


+/// ParseCXXSimpleTypeSpecifier - [C++ 7.1.5.2] Simple type specifiers.

Please add a comment saying that this should only be called when the  
current token is known to be part of a simple type specifier.  While  
this is somewhat implicit in the rest of the actions, for some reason  
I had to check for this one when I saw the identifier handling code.

+  // GNU typeof support.
+  case tok::kw_typeof:
+    ParseTypeofSpecifier(DS);
+    DS.Finish(Diags, PP.getSourceManager(), getLang());
+    return;
+  }

This doesn't call DS.SetRangeEnd?  Does ParseTypeofSpecifier do this?


+    // FIXME: Support for class constructors.
+    assert(0 && "No support for class constructors yet.");
+    abort();

Please make this output an error (e.g. unimplemented), it is nice for  
clang to never crash on any input, even if due to a partially  
implemented one.


+  // C++ 5.2.3p1:
+  // If the expression list is a single expression, the type conversion
+  // expression is equivalent (in definedness, and if defined in  
meaning) to the
+  // corresponding cast expression.

It seems that this check should happen before the check for recorddecl?



>
> Ambiguity introduced by function-style casting, will be resolved in  
> another patch.

What ambiguity is that?

-Chris