[PATCH] clang-format properly handles try/catch blocks.

[Daniel Jasper]

Could you generally please attach patches to emails? That makes it much
easier to ensure that one is looking at the same version and also to apply
the patch to a local client.

I just have a few higher level comments and will leave comments to the
parser part to Manuel.

On Tue, Jan 14, 2014 at 9:32 PM, Diego Alexander Rojas <
alexander.rojas at gmail.com> wrote:

> I just completed the support for function-try blocks


> --
> Diego Alexander Rojas Páez
> -------------- next part --------------
> Index: lib/Format/UnwrappedLineParser.cpp
> ===================================================================
> --- lib/Format/UnwrappedLineParser.cpp (revision 199220)
> +++ lib/Format/UnwrappedLineParser.cpp (working copy)
> @@ -621,6 +621,9 @@
>    case tok::kw_private:
>      parseAccessSpecifier();
>      return;
> +  case tok::kw_try:
> +    parseTryCatch();
> +    return;
>    case tok::kw_if:
>      parseIfThenElse();
>      return;
> @@ -708,6 +711,10 @@
>        // Otherwise this was a braced init list, and the structural
>        // element continues.
>        break;
> +    case tok::kw_try:
> +      // It enters here in function-try blocks
>

Maybe:
// This is a function try block.


> +      parseTryCatch();
> +      return;
>      case tok::identifier: {
>        StringRef Text = FormatTok->TokenText;
>        nextToken();
> @@ -1028,6 +1035,65 @@
>    }
>  }
>
> +void UnwrappedLineParser::parseTryCatch() {
> +  assert(FormatTok->Tok.is(tok::kw_try) && "'try' expected");
> +  nextToken();
> +  bool NeedsUnwrappedLine = false;
> +  if (FormatTok->Tok.is(tok::colon)) {
> +      // We are in a function try block, what comes is an initializer list
>

Maybe:
// This starts the initializer list of a constructor try block.


> +      nextToken();
> +      while (FormatTok->Tok.is(tok::identifier)) {
> +          nextToken();
> +          if (FormatTok->Tok.is(tok::l_paren))
> +            parseParens();
> +          else
> +            StructuralError = true;
> +          if (FormatTok->Tok.is(tok::comma))
> +              nextToken();
>

Preferably, we'd like test cases for all of the codepaths here.


> +      }
> +  }
> +  if (FormatTok->Tok.is(tok::l_brace)) {
> +    CompoundStatementIndenter Indenter(this, Style, Line->Level);
> +    parseBlock(/*MustBeDeclaration=*/false);
> +    if (Style.BreakBeforeBraces == FormatStyle::BS_Allman ||
> +        Style.BreakBeforeBraces == FormatStyle::BS_GNU ||
> +        Style.BreakBeforeBraces == FormatStyle::BS_Stroustrup) {
> +      addUnwrappedLine();
> +    } else {
> +      NeedsUnwrappedLine = true;
> +    }
> +  } else if (!FormatTok->Tok.is(tok::kw_catch)) {
> +    // The C++ standard requires a compound-statement after a try.
>

Do you mean "catch"-statement?


> +    // If there's none, we try to assume there's a structuralElement
> +    // and try to continue.
> +    StructuralError = true;
> +    addUnwrappedLine();
> +    ++Line->Level;
> +    parseStructuralElement();
> +    --Line->Level;
> +  }
> +  while (FormatTok->Tok.is(tok::kw_catch)) {
> +    nextToken();
> +    if (FormatTok->Tok.is(tok::l_paren))
> +      parseParens();
> +    NeedsUnwrappedLine = false;
> +    if (FormatTok->Tok.is(tok::l_brace)) {
> +      CompoundStatementIndenter Indenter(this, Style, Line->Level);
> +      parseBlock(/*MustBeDeclaration=*/false);
> +      if (Style.BreakBeforeBraces == FormatStyle::BS_Allman ||
> +          Style.BreakBeforeBraces == FormatStyle::BS_GNU ||
> +          Style.BreakBeforeBraces == FormatStyle::BS_Stroustrup) {
> +        addUnwrappedLine();
> +      } else {
> +        NeedsUnwrappedLine = true;
> +      }
> +    }
> +  }
> +  if (NeedsUnwrappedLine) {
> +    addUnwrappedLine();
> +  }
>

This needs a test case with multiple catch blocks.


> +}
> +
>  void UnwrappedLineParser::parseNamespace() {
>    assert(FormatTok->Tok.is(tok::kw_namespace) && "'namespace' expected");
>    nextToken();
> Index: lib/Format/UnwrappedLineParser.h
> ===================================================================
> --- lib/Format/UnwrappedLineParser.h (revision 199220)
> +++ lib/Format/UnwrappedLineParser.h (working copy)
> @@ -85,6 +85,7 @@
>    void parseReturn();
>    void parseParens();
>    void parseSquare();
> +  void parseTryCatch();
>    void parseIfThenElse();
>    void parseForOrWhileLoop();
>    void parseDoWhile();
> Index: unittests/Format/FormatTest.cpp
> ===================================================================
> --- unittests/Format/FormatTest.cpp (revision 199220)
> +++ unittests/Format/FormatTest.cpp (working copy)
> @@ -1810,15 +1810,11 @@
>  }
>
>  TEST_F(FormatTest, FormatTryCatch) {
> -  // FIXME: Handle try-catch explicitly in the UnwrappedLineParser, then
> we'll
> -  // also not create single-line-blocks.
>    verifyFormat("try {\n"
>                 "  throw a * b;\n"
> -               "}\n"
> -               "catch (int a) {\n"
> +               "} catch (int a) {\n"
>                 "  // Do nothing.\n"
> -               "}\n"
> -               "catch (...) {\n"
> +               "} catch (...) {\n"
>                 "  exit(42);\n"
>                 "}");
>
> @@ -2271,9 +2267,8 @@
>              "  f(x)\n"
>              "  try {\n"
>              "    q();\n"
> +            "  } catch (...) {\n"
>              "  }\n"
> -            "  catch (...) {\n"
> -            "  }\n"
>              "}\n",
>              format("int q() {\n"
>                     "f(x)\n"
> @@ -2288,8 +2283,7 @@
>    EXPECT_EQ("class A {\n"
>              "  A() : t(0) {}\n"
>              "  A(X x)\n" // FIXME: function-level try blocks are broken.
> -            "  try : t(0) {\n"
> -            "  }\n"
> +            "  try : t(0) {}\n"
>              "  catch (...) {\n"
>              "  }\n"
>              "};",
> @@ -7179,9 +7173,8 @@
>  TEST_F(FormatTest, CatchExceptionReferenceBinding) {
>    verifyFormat("void f() {\n"
>                 "  try {\n"
> +               "  } catch (const Exception &e) {\n"
>                 "  }\n"
> -               "  catch (const Exception &e) {\n"
> -               "  }\n"
>                 "}\n",
>                 getLLVMStyle());
>
 }
>

We need tests for the different style options. Comment #10 on
llvm.org/PR18418 seems like a good set to start with.

Many thanks for working on this!
Daniel

_______________________________________________
> cfe-commits mailing list
> cfe-commits at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20140114/722a0800/attachment.html>