Formatting using Stroustrup brace breaking breaks try/catch blocks

[llvmbot]

Bugzilla Link	18418
Resolution	DUPLICATE
Resolved on	Aug 09, 2014 14:25
Version	trunk
OS	All
Reporter	LLVM Bugzilla Contributor

Extended Description

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.

[llvmbot]

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.

[llvmbot]

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

[llvmbot]

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

[llvmbot]

Added a parseTryCatch method on the UnwrappedLine to actually handle try/catch blocks
So, I checked and the source of the problem was that try/catch blocks weren't handled at all. This patch seems to work, but I am not sure how to handle situations where the input is invalid, i.e. after either try or catch there not a compound-statement but a simple statement.

The patch also doesn't handle the obscure function-try-block, because I didn't even knew they existed until today. So I wanted to know if the patch is enough before sending it to cfe-commits

[llvmbot]

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

[llvmbot]

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.

[llvmbot]

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.

[llvmbot]

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

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

[llvmbot]

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

[llvmbot]

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.

[llvmbot]

Those look right to me.

[llvmbot]

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

[llvmbot]

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/

[llvmbot]

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 llvm/llvm-bugzilla-archive#19016 ***

[llvmbot]

mentioned in issue llvm/llvm-bugzilla-archive#19016