Issue769

Title Uncrustify unable to fix formatting
Priority bug Status resolved
Superseder Nosy List florian, jendrik, malte, patfer
Assigned To Keywords infrastructure
Optional summary

Created on 2018-03-16.20:53:58 by patfer, last changed by malte.

Files
File name Uploaded Type Edit Remove
global_operator.cc patfer, 2018-03-16.20:53:58 text/x-c++src
Messages
msg6939 (view) Author: malte Date: 2018-03-16.21:31:07
In the interest of using our own resources wisely, I'm happy to leave this as is
on our side. :-) (Marking as resolved.)
msg6938 (view) Author: patfer Date: 2018-03-16.21:27:24
I take a look into testing this with the most current version and depending on
the complexity of their submission system submit it.

Shall I mark this as resolved and give it to the uncrustify developers or do you
want to write your own workaround (e.g. inform the user if it was unable to
correct the formatting)? I would close it, unable to correct it does not have
harmful effects (except me wondering three times why my pipeline fails).

Do not worry, I would not actively write such code, it happend due to a
refactoring and was changed after detection.
msg6936 (view) Author: malte Date: 2018-03-16.21:11:29
Interesting case. Yes, I think marking such things as bugs (or some other
category) in the tracker makes sense. If you want you can use the
"infrastructure" keyword to mark that it is not about the code itself, but I'm
not sure how consistent we are with using the keywords. (Keywords are taken from
a fixed set and mainly useful for saved queries and things like that.)


hg uncrustify does actually recommend changing something (due to its
architecture, it cannot be internally inconsistent about complaining in general
vs. recommending a concrete change; you will see why if you check how the three
modes of status/diff/modify of "hg uncrustify" are implemented), but what it
recommend makes no sense. It wants to insert a blank line. And if you do that,
it wants to insert another blank line, etc. (Try "hg uncrustify --diff", and you
will see the "+" indicating an added blank line.)

If you want, you can submit a bug report to the uncrustify project (but that
requires first trying with their latest version, I guess), but for our purposes
a simpler fix is to change the formatting.

We generally follow the rule "If an inner block has braces, the outer block
should also have braces." It's not documented on the wiki; we generally only
document the things that cause problems multiple times to avoid the coding
conventions documentations becoming too long to read. So in this example, we
would use

if (CND1) {
    if (CND2) {
        CODE;
    }
}

(A version without braces is generally also possible, but not here because CODE
has multiple statements.)
msg6935 (view) Author: patfer Date: 2018-03-16.20:53:58
(I do not know if this is an issue on our script or on the uncrustify executable)

Attached is a file which uncrustify does not accept, but at the same time, it
does neither change anything nor complain that it is unable to fix the formating.

The problematic code is:
if (CND1)
    if (CND2) {
      CODE
    }

The first if condition is has no brackets.

(Actually this is a bug, but one in the formatting and quite unimportant. Should
I have marked it as bug?)
History
Date User Action Args
2018-03-16 21:31:07maltesetstatus: chatting -> resolved
messages: + msg6939
2018-03-16 21:27:24patfersetpriority: wish -> bug
messages: + msg6938
keyword: + infrastructure
2018-03-16 21:11:29maltesetstatus: unread -> chatting
messages: + msg6936
2018-03-16 20:53:58patfercreate