Formatting using Stroustrup brace breaking breaks try/catch blocks

[Quuxplusone]

Bugzilla Link	PR18418
Status	RESOLVED DUPLICATE of bug 19016
Importance	P normal
Reported by	Alex (alexander.rojas@gmail.com)
Reported on	2014-01-08 04:00:47 -0800
Last modified on	2014-08-09 14:25:25 -0700
Version	trunk
Hardware	PC All
CC	alexander.rojas@gmail.com, djasper@google.com, jibz-llvmbugs@stdip.com, klimek@google.com, romankashicin@gmail.com
Fixed by commit(s)
Attachments	18418.patch (2490 bytes, text/plain)
Blocks
Blocked by
See also

Not sure if this is a bug or the style is supposed to be like that, but the
breaking of the braces in try/catch blocks is not consistent with the one for
the other blocks.

For example one would expect

try {
    ...
} catch (std::exception &e) {
    ...
}

but obtains

try
{
    ...
}
catch (std::exception &e)
{
    ...
}

While at the same time other blocks are formatted properly.

[Quuxplusone]

I've been checking the code, doesn't seem to difficult to generate a fix, so if you guys accept that this need to be change I can take care of it.

[Quuxplusone]

Sure, just send the patch to cfe-commits and we'll review it.

[Quuxplusone]

Sure, I'm personally not using stroustrup braces so I trust you on this. Generally, patches very welcome :)

[Quuxplusone]

Attached 18418.patch (2490 bytes, text/plain): Added a parseTryCatch method on the UnwrappedLine to actually handle try/catch blocks

[Quuxplusone]

It needs tests (unittests/Format/FormatTest.cpp) but otherwise seems like a good start. Manuel and others might/will weigh in on details once the patch is sent to cfe-commits@ (that is our usual platform for code reviews).

[Quuxplusone]

If the book "The C++ Programming Language" is any kind of reference for the
Stroustrup style, then it looks like try/catch is formatted like if/else, which
is with opening braces attached, and the catch broken from the closing brace:

try {
    // something
}
catch {
    // something
}

See for instance the chapter on exceptions.

[Quuxplusone]

I just check the book and you seem to be right, so it seems my company has used
the style wrong for far too long (we base our code on the formatting made by
astyle, which I just checked is wrong in this case too).

But this leads me to the question: which one should be the formatting in the
other styles? The one produced by attach is also

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

but I am inclined to think it should be

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

Also I think the result produce by clang-format with GNU is wrong, it produces

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

when I think it should be

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

Which this patch formats correctly. In this case, I think I should change the
patch but even the comments in the FormatTest.cpp suggest we should handle the
try/catch blocks in the way I suggest.

[Quuxplusone]

I would also say that for other styles, it should be:

try {
    ...
} catch (std::exception &e) {
    ...
}

[Quuxplusone]

I don't know what exactly "attach" covers, but I agree it would make sense to
attach the braces on try/catch for that style.

The problem with GNU style [1] (like K&R) is that it (to my knowledge) only
covers C, so the formatting of C++ is perhaps a bit subjective. But again, I
think it makes sense to follow the formatting of if/else, which seems to be
what GNU projects like GCC do [2].

Clang-format does not follow Stroustrup's style for if/else either, I submitted
a bug report for that [3].

On a side note, astyle does have an option (--break-closing-brackets) that
works for if/else, but it breaks all constructs including do/while, which
should not be (in my personal opinion).

[1]: http://www.gnu.org/prep/standards/html_node/Formatting.html
[2]: http://gcc.gnu.org/onlinedocs/libstdc++/manual/source_code_style.html
[3]: http://llvm.org/bugs/show_bug.cgi?id=18446

[Quuxplusone]

To summarize, we should agree on the current brace breaking for each case.

for attach and Linux it should be:

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

for Stroustrup:
try {
    // something
}
catch (...) {
    // something
}

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

And GNU:
try
    {
        // something
    }
catch (...)
    {
        // something
    }

If we can settle on that, I will do the changes to the patch.

On a different note, I actually would like if there were more granular control
over how one wants to do the brace braking, something similar to how eclipse
does it for java where one can decide how to behave in the presence of each
control statement.

[Quuxplusone]

Those look right to me.

[Quuxplusone]

I just send the patch to cfe-commits. Once it is accepted this ticket can be closed.

[Quuxplusone]

Thanks, hope it goes through.

Since you mentioned astyle, I wanted to add that I submitted a patch there for the same issue; https://sourceforge.net/p/astyle/bugs/267/

[Quuxplusone]

I think it's the same problem as in http://llvm.org/bugs/show_bug.cgi?id=19016.

It's fixed in revision 208302.

This bug has been marked as a duplicate of bug 19016