clang-format properly handles try/catch blocks

[sambatyon]

Summary

The patch adds a method so try/catch blocks are handled explicitly.

It generates the right formatting in all cases.

the generated output for Attach and Linux is now:

try {
    // something
} catch (...) {
    // something
}

for Stroustrup:

try {
    // something
}
catch (...) {
    // something
}

Allman:

try
{
    // something
}
catch (...)
{
    // something
}

And GNU:

try
    {
        // something
    }
catch (...)
    {
        // something
    }

[sambatyon]

Just added the whole context

[klimek]

Overall, I'm not sure whether we can basically just parse:
try ... {<block>} catch ... {<block} ...
where we just eat away anything between the try/catch and the opening brace...

lib/Format/UnwrappedLineParser.cpp

715

Perhaps: "We arrive here when parsing function-try blocks."

1042

I'm wondering whether we just want to eat everything up to the next '{'. In general, we try to parse as little as possible in the structural parser.

If we decide we really need to parse the structure here, I think we should pull out a parseInitializerList() method.

1043

Nit: add '.'.

1076

Same here. The problem is if anybody ever finds a real use case to put anything in between (including macros) we might fail to parse the file...

[djasper]

Also please take a look at the comments I had on the original mail.

[sambatyon]

lib/Format/UnwrappedLineParser.cpp

1042

I've been thinking on how to parse the initializer list, and it might be quite cumbersome. I've been thinking on leaving out of this patch the ability of dealing with function-try blocks and leave it for latter (since I do this as a hobby, there's really a limit on how much time I can spend on this).

1043

Sorry, but what do you mean by “Nit”?

1075–1091

From Daniel: This needs a test case with multiple catch blocks.

1076

I don't get what you mean here. If the token is not tok::kw_catch, it won't start the while and NeedsUnwrappedLine will be true. Is there any other case that needs consideration?

[klimek]

lib/Format/UnwrappedLineParser.cpp

1042

I think we're misunderstanding each other here... What I mean is that we should parse *less* accurately. The thing to remember is that clang-format must work well on incorrect code. Thus, the structural parser tries hard to find only the structural elements that matter.

Regarding the time limits you have: especially when you do this as a hobby, I'd say this is the perfect venue to try to get things "right". As nobody forces you to do it, there's also no deadline pressure, so you can take all the time you need... The worst that can happen is that somebody asks you whether they can take over and finish the patch if they need it more urgently.

1043

A minor comment that's not really important :)

1076

Well, what do we do on this snippet:

...
} catch (Type) ANNOTATION_MACRO(something else) {
   ...
}

As I said, I'm not sure exactly how to resolve this.

Daniel, do you have ideas here?

[djasper]

lib/Format/UnwrappedLineParser.cpp

1076

Maybe we should just consume everything until the next "{" (possibly recognizing a few error cases explicitly - e.g. ";" or "}").

[djasper]

Hi. Are you moving forward with this? Somebody else has also started a similar patch: http://llvm-reviews.chandlerc.com/D2731.

[sdt]

lib/Format/UnwrappedLineParser.cpp

1042

I don't think you can just consume until the next brace, due to brace initialization:

try : foo{}, bar{nullptr} {} catch (...) {}

[sdt]

lib/Format/UnwrappedLineParser.cpp

1042

klimek, when you say that you'd like to have parseInitializerList() broken out, do you mean for it to be used anywhere else, or just to pull it out to make it cleaner here and in potential future uses? I couldn't find anywhere we explicitly parse initializer lists already.

[sambatyon]

lib/Format/UnwrappedLineParser.cpp

1042

I actually had an implementation of that and then while writing a test I realized that didn't work. The best option is to actually parse the initializer list.

[sambatyon]

I think I will let sdt take care of this.

[djasper]

I addressed the comments, added tests and submitted this as r208302. Thank you!

lib/Format/UnwrappedLineParser.h

private:
   void parsePPEndIf();
   void parsePPUnknown();
   void parseStructuralElement();
   bool tryToParseBracedList();
   bool parseBracedList(bool ContinueOnSemicolons = false);
   void parseReturn();
   void parseParens();
   void parseSquare();
+  void parseTryCatch();
   void parseIfThenElse();
   void parseForOrWhileLoop();
   void parseDoWhile();
   void parseLabel();
   void parseCaseLabel();
   void parseSwitch();
   void parseNamespace();
   void parseAccessSpecifier();

lib/Format/UnwrappedLineParser.cpp

if (FormatTok->Tok.is(tok::kw_namespace)) {
       return;
     }
     break;
   case tok::kw_public:
   case tok::kw_protected:
   case tok::kw_private:
     parseAccessSpecifier();
     return;
+  case tok::kw_try:
+    parseTryCatch();
+    return;
   case tok::kw_if:
     parseIfThenElse();
     return;
   case tok::kw_for:
   case tok::kw_while:
     parseForOrWhileLoop();
     return;
   case tok::kw_do:
case tok::l_brace:
         FormatTok->Type = TT_FunctionLBrace;
         parseBlock(/*MustBeDeclaration=*/false);
         addUnwrappedLine();
         return;
       }
       // Otherwise this was a braced init list, and the structural
       // element continues.
       break;
+    case tok::kw_try:
+      // It enters here in function-try blocks
+      parseTryCatch();
+      return;
     case tok::identifier: {
       StringRef Text = FormatTok->TokenText;
       nextToken();
       if (Line->Tokens.size() == 1) {
         if (FormatTok->Tok.is(tok::colon)) {
           parseLabel();
           return;
         }
if (FormatTok->Tok.is(tok::l_brace)) {
       parseStructuralElement();
       --Line->Level;
     }
   } else if (NeedsUnwrappedLine) {
     addUnwrappedLine();
   }
 }
 
+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
+      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();
+      }
+  } 
+  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.
+    // 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();
+  }
+}
+
 void UnwrappedLineParser::parseNamespace() {
   assert(FormatTok->Tok.is(tok::kw_namespace) && "'namespace' expected");
   nextToken();
   if (FormatTok->Tok.is(tok::identifier))
     nextToken();
   if (FormatTok->Tok.is(tok::l_brace)) {
     if (Style.BreakBeforeBraces == FormatStyle::BS_Linux ||
         Style.BreakBeforeBraces == FormatStyle::BS_Allman ||

unittests/Format/FormatTest.cpp

verifyFormat(
       "asm(\"movq\\t%%rbx, %%rsi\\n\\t\"\n"
       "    \"cpuid\\n\\t\"\n"
       "    \"xchgq\\t%%rbx, %%rsi\\n\\t\"\n"
       "    : \"=a\"(*rEAX), \"=S\"(*rEBX), \"=c\"(*rECX), \"=d\"(*rEDX)\n"
       "    : \"a\"(value));");
 }
 
 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"
                "}");
 
   // Function-level try statements.
   verifyFormat("int f() try { return 4; }\n"
                "catch (...) {\n"
                "  return 5;\n"
                "}");
EXPECT_EQ("int q() {\n"
             "  f(x)\n"
             "  while (1) {\n"
             "  }\n"
             "  f(x)\n"
             "  g(x);\n"
             "  f(x)\n"
             "  try {\n"
             "    q();\n"
-            "  }\n"
-            "  catch (...) {\n"
+            "  } catch (...) {\n"
             "  }\n"
             "}\n",
             format("int q() {\n"
                    "f(x)\n"
                    "if (1) {}\n"
                    "f(x)\n"
                    "while (1) {}\n"
                    "f(x)\n"
                    "g(x);\n"
                    "f(x)\n"
                    "try { q(); } catch (...) {}\n"
                    "}\n"));
   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"
             "};",
             format("class A {\n"
                    "  A()\n : t(0) {}\n"
                    "  A(X x)\n"
                    "  try : t(0) {} catch (...) {}\n"
                    "};"));
verifyFormat("enum X\n"
                "{\n"
                "  Y = 0,\n"
                "}\n",
                GNUBraceStyle);
 }
 TEST_F(FormatTest, CatchExceptionReferenceBinding) {
   verifyFormat("void f() {\n"
                "  try {\n"
-               "  }\n"
-               "  catch (const Exception &e) {\n"
+               "  } catch (const Exception &e) {\n"
                "  }\n"
                "}\n",
                getLLVMStyle());
 }
 
 TEST_F(FormatTest, UnderstandsPragmas) {
   verifyFormat("#pragma omp reduction(| : var)");
   verifyFormat("#pragma omp reduction(+ : var)");