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

Manuel Klimek klimek at google.com
Tue Jan 14 23:16:56 PST 2014


On Tue, Jan 14, 2014 at 9:53 PM, Daniel Jasper <djasper at google.com> wrote:

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

For reviews from me you can also make my life significantly easier (because
I don't need to first patch it in locally) if you use
http://llvm-reviews.chandlerc.com (docs at
http://llvm.org/docs/Phabricator.html).

Cheers,
/Manuel


>
> 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/20140115/85dbd765/attachment.html>


More information about the cfe-commits mailing list