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

[Argiris Kirtzidis]

Chris Lattner wrote:
>
> +/// 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. :)

Done.

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

This looks great! I've made CastExpr an abstract class and replaced its 
use with an ExplicitCastExpr here:
http://lists.cs.uiuc.edu/pipermail/cfe-commits/Week-of-Mon-20080818/007043.html

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

Done.

>
>  /// [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?

No, they work fine. Here are some comments from ParseCastExpression:
>   // This handles all of cast-expression, unary-expression, 
> postfix-expression,
>   // and primary-expression.  We handle them together like this for 
> efficiency
>   // and to simplify handling of an expression starting with a '(' token

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

Done.

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

Done.

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

Done.

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

Yes, ParseTypeofSpecifier does it. I've made a small commit, before 
posting the patch, here:
http://lists.cs.uiuc.edu/pipermail/cfe-commits/Week-of-Mon-20080811/007003.html

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

Done.

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

My intention was to do something like this:
if class type
    -> do semantic checks, resolve constructors etc.
    -> if single expr
              -> create some class-specialized expr
        else if multi expr
              -> create some other class-specialized expr
 ......


Instead of:
if single expr
        -> if non-class type
                   -> create CXXFunctionalCastExpr
        -> else if class type
                    -> semantic checks, resolve constructor
                    -> create some class-specialized expr

Not a big difference though. Will probably be more clear what is better 
when constructors are in place and it's determined what kind of Exprs 
are needed to represent them.


New patch attached!


-Argiris
-------------- next part --------------
A non-text attachment was scrubbed...
Name: func-style-cast-2.patch
Type: text/x-diff
Size: 23181 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-dev/attachments/20080818/a2ed4c0b/attachment.patch>