parse C++ throw

Here is a patch to add parsing support for C++ throw expressions. I tested to ensure this would be safe for C, it is, as throw isn't only recognized when doing C++. Did I do the type of the throw expression correctly? I copied the way true and false were done.

eh-1.diffs (6.79 KB)

Here is a patch to add parsing support for C++ throw expressions.

Wow, very nice. You did a great job, I just have some minor nit-picky comments below.

I tested to ensure this would be safe for C, it is, as throw isn't only recognized when doing C++.

Right!

Did I do the type of the throw expression correctly? I copied the way true and false were done.

I'm surprised that throw is an expression, you learn something new every day. Your code looks great.

+/// PraseThrowExpression - THis handles the C++ throw expression.

Minor typos here: prase -> parse, THis -> This

Tabs:

+ virtual ExprResult ActOnCXXThrow(SourceLocation OpLoc,
+ ExprTy *Op = 0) {

+ /// CXXThrowExpr - [C++ 15] C++ Throw Expression.
+ ///
+ class CXXThrowExpr : public Expr {
+ Expr *Op;
+ SourceLocation Loc;

I'd suggest renaming Loc -> ThrowLoc to be more explicit about what it is the location of. In the comment, please say explicitly that this handles two cases "throw;" in which case the operand is null, and "throw x;" in which case x is the operand.

This method:
+ Expr *getSubExpr() const { return Op; }

Should be replaced with these two:
+ const Expr *getSubExpr() const { return Op; }
+ Expr *getSubExpr() { return Op; }

Please resubmit the tweaked patch and I'll apply, thanks Mike!

-Chris

I'll fix mine... but, this cleans up the rest of clang:

tabs-1.diffs (17.7 KB)

Nice, thanks!
http://lists.cs.uiuc.edu/pipermail/cfe-commits/Week-of-Mon-20080225/004428.html

-Chris

find . -name \*.h -o -name \*.cpp | xargs grep -c ' ' | grep -v ':0$'
./AST/Decl.cpp:46
./Driver/ASTConsumers.cpp:5
./Driver/clang.cpp:1
./include/clang/AST/ASTContext.h:1
./include/clang/AST/Decl.h:3
./include/clang/AST/DeclObjC.h:13
./include/clang/Parse/Action.h:3
./include/clang/Parse/Parser.h:2
./Parse/ParseDecl.cpp:1
./Parse/ParseExprCXX.cpp:3
./Sema/Sema.h:1
./Sema/SemaDecl.cpp:5
./Sema/SemaDeclObjC.cpp:1
./Sema/SemaExprObjC.cpp:4

:frowning: Apparently I'm not the only one. Do you think the number of people that miss set their physical tabs is larger than the number of people that use emacs? Why not punish the people with wrong/weird tabs settings over punishing people that use emacs? After all, we can merely educate people to use softtabstop and shiftwidth appropriately.

If not that... could we add:
   /* -*- indent-tabs-mode:nil; -*- */
the tops of all the files or find some other way reduce the burden for emacs users?

Catering to people that think tabs in source files mean something other than 8, is like catering to people that like 6 bit characters or 60 bit words, I'd rather just laugh at them. Burdening all developers to `do something' doesn't strike me as friendly.

:frowning: Apparently I'm not the only one. Do you think the number of people that miss set their physical tabs is larger than the number of people that use emacs? Why not punish the people with wrong/weird tabs settings over punishing people that use emacs? After all, we can merely educate people to use softtabstop and shiftwidth appropriately.

If not that... could we add:
/* -*- indent-tabs-mode:nil; -*- */
the tops of all the files or find some other way reduce the burden for emacs users?

Catering to people that think tabs in source files mean something other than 8, is like catering to people that like 6 bit characters or 60 bit words, I'd rather just laugh at them. Burdening all developers to `do something' doesn't strike me as friendly.

I don't know what emacs has to do with this discussion. If you're confused about why tabs in source files is considered to be a bad thing by many (but not all!) people, there are extensive discussions of the topic available on the interweb.

-Chris

Hello, Mike

If not that... could we add:
   /* -*- indent-tabs-mode:nil; -*- */
the tops of all the files or find some other way reduce the burden for
emacs users?

Why don't add something like this to .emacs? :

(add-hook 'c-mode-hook
          (function
      (lambda nil
        (if (string-match "llvm" buffer-file-name)
      (progn
        (c-set-style "LLVMCodeStyle")
        )
          ))))

I'm successfully using this approach for almost transparent switch over
different codestyles for several projects.

+/// PraseThrowExpression - THis handles the C++ throw expression.

Minor typos here: prase -> parse, THis -> This

Fixed. Found one more instance of prase, fixed that as well.

Tabs:

+ virtual ExprResult ActOnCXXThrow(SourceLocation OpLoc,
+ ExprTy *Op = 0) {

Fixed all tabs.

+ /// CXXThrowExpr - [C++ 15] C++ Throw Expression.
+ ///
+ class CXXThrowExpr : public Expr {
+ Expr *Op;
+ SourceLocation Loc;

I'd suggest renaming Loc -> ThrowLoc

Sure... Fixed.

In the comment, please say explicitly that this handles two cases "throw;" in which case the operand is null, and "throw x;" in which case x is the operand.

Done. I added some more comments as well.

This method:
+ Expr *getSubExpr() const { return Op; }

Should be replaced with these two:
+ const Expr *getSubExpr() const { return Op; }
+ Expr *getSubExpr() { return Op; }

I just copied CXXCastExpr, which didn't have it. I'll submit a patch for that next.

I also added a FIXME for handling throw when not followed by a ';' nor an assignment-expression. Something parser generators do automagically for us, but something we have to compute. Do we have a tentative parse system yet or some other easy way to do this? [ I think I know the answer, I bet not. ]

eh-1a.diffs (8.15 KB)

I just copied CXXCastExpr, which didn't have it. I'll submit a patch for that next.

Thanks Mike!

Ok. A couple new things:

--- ./Parse/ParseExprCXX.cpp.~1~ 2008-02-25 13:35:37.000000000 -0800
+++ ./Parse/ParseExprCXX.cpp 2008-02-25 13:35:57.000000000 -0800
@@ -1,3 +1,4 @@
+/* -*- indent-tabs-mode:nil; -*- */

Please don't do this.

+ class CXXThrowExpr : public Expr {
...
+ virtual SourceRange getSourceRange() const {
+ return SourceRange(ThrowLoc, getSubExpr()- >getSourceRange().getEnd());
+ }

If getSubExpr() is null, this should return SourceRange(ThrowLoc, ThrowLoc).

+++ ./AST/StmtPrinter.cpp 2008-02-25 13:35:57.000000000 -0800
@@ -780,6 +780,12 @@ void StmtPrinter::VisitCXXBoolLiteralExp
    OS << (Node->getValue() ? "true" : "false");
  }

+void StmtPrinter::VisitCXXThrowExpr(CXXThrowExpr *Node) {
+ OS << "throw ";
+ if (Node->getSubExpr())
+ PrintExpr(Node->getSubExpr());

It would be slightly nicer to not print the space after the throw if subexpr is null.

I also added a FIXME for handling throw when not followed by a ';' nor an assignment-expression. Something parser generators do automagically for us, but something we have to compute. Do we have a tentative parse system yet or some other easy way to do this? [ I think I know the answer, I bet not. ]

The fixme is fine for a first step. However, it would be better to eventually add a predicate that determines whether a token is the start of an expression. I don't think there are any cases where a declaration is allowed after a throw, so this predicate should be relatively straight-forward, something like Parser::isDeclarationSpecifier().

I agree this is somewhat ugly, but it could be worse

-Chris

+/* -*- indent-tabs-mode:nil; -*- */

Please don't do this.

FIxed.

+ class CXXThrowExpr : public Expr {
...
+ virtual SourceRange getSourceRange() const {
+ return SourceRange(ThrowLoc, getSubExpr()->getSourceRange().getEnd());
+ }

If getSubExpr() is null, this should return SourceRange(ThrowLoc, ThrowLoc).

Ah, yes, of course. Fixed.

+++ ./AST/StmtPrinter.cpp 2008-02-25 13:35:57.000000000 -0800
@@ -780,6 +780,12 @@ void StmtPrinter::VisitCXXBoolLiteralExp
  OS << (Node->getValue() ? "true" : "false");
}

+void StmtPrinter::VisitCXXThrowExpr(CXXThrowExpr *Node) {
+ OS << "throw ";
+ if (Node->getSubExpr())
+ PrintExpr(Node->getSubExpr());

It would be slightly nicer to not print the space after the throw if subexpr is null.

Yeah, I thought about doing that when I first implemented it and decided against worrying about it as it didn't seem as bad as the extra {} in extern "C" handling. But, it is easy, localized and doesn't require any more data in the AST to do it, sooo.... Fixed.

I also added a FIXME for handling throw when not followed by a ';' nor an assignment-expression. Something parser generators do automagically for us, but something we have to compute. Do we have a tentative parse system yet or some other easy way to do this? [ I think I know the answer, I bet not. ]

The fixme is fine for a first step. However, it would be better to eventually add a predicate that determines whether a token is the start of an expression.

Agreed.

I don't think there are any cases where a declaration is allowed after a throw,

You mean like this:

   throw int(1);

? Yup, that's valid.

so this predicate should be relatively straight-forward, something like Parser::isDeclarationSpecifier().

I agree this is somewhat ugly, but it could be worse

Just wait, we'll get there.

eh-1a.diffs (7.53 KB)

+void StmtPrinter::VisitCXXThrowExpr(CXXThrowExpr *Node) {
+ OS << "throw ";
+ if (Node->getSubExpr())
+ PrintExpr(Node->getSubExpr());

It would be slightly nicer to not print the space after the throw if subexpr is null.

Yeah, I thought about doing that when I first implemented it and decided against worrying about it as it didn't seem as bad as the extra {} in extern "C" handling. But, it is easy, localized and doesn't require any more data in the AST to do it, sooo.... Fixed.

Yeah I agree that it's not a big issue :slight_smile:

I don't think there are any cases where a declaration is allowed after a throw,

You mean like this:

throw int(1);

? Yup, that's valid.

yuck. Ok.

so this predicate should be relatively straight-forward, something like Parser::isDeclarationSpecifier().

I agree this is somewhat ugly, but it could be worse

Just wait, we'll get there.

Applied, thanks Mike!
http://lists.cs.uiuc.edu/pipermail/cfe-commits/Week-of-Mon-20080225/004439.html

-Chris