Rawtherapee: astyle rt code

[heckflosse]

In my latest try to astyle lcp code, astyle failed miserably.
How can we improve astyle for better formatting?

@Floessie @Beep6581 @Hombre57 @Desmis ?

[Beep6581]

Maybe one of these options will satisfy your needs:
http://astyle.sourceforge.net/astyle.html#_pad-oper

[Beep6581]

@Floessie I recall someone, probably you, mentioning somewhere a better solution than astyle. Could you let me know what was the name of that tool?

[Floessie]

@Beep6581 Sure, it's uncrustify. But be warned: It has nearly 600 options ATM. :wink:

[Floessie]

@Beep6581 @heckflosse @Hombre57 @Desmis @agriggio As a first improvement, I'd like to propose to pick up our consent from #4010 that pad-first-paren-out was a bad idea and give unpad-paren a try, as mentioned here. I must admit, I don't have the guts to dictate a style for all of us, so I won't be the one to change our astylerc, but I hesitate to run astyle with the current settings on my code, because I dislike the "space before call" so much.

Please share your opinions.

BTW: uncrustify really is a great tool, as you can configure almost every aspect. But that's also its greatest drawback: It takes hours to tweak it to the way you like, and even then it sometimes misses the goal because there are dependencies hard to work around.

[agriggio]

well I'm generally against automatic formatting, and really dislike the results of astyle as configured now in RT. so I'm in favour of any change to it :-)
my ideal solution would be to come up with some mild conventions about names and not be too picky with indentation as long as it is consistent within files.
however, I can also live with the current situation -- imho it makes life harder rather than easier, but it's not unbearable...

[Beep6581]

My opinion is that your opinion is the right opinion.

Automatically formatting the RT code made most of it more readable, as it was a soup of various styles and indentations before, including simply wrong indentation. IIRC I did that when we switched from Google Code to GitHub. It's not so important to use it now, though it does enforce a certain level of consistency. I would just like the style to suite you.

[Beep6581]

We could run astyle through all files after procparams-cleanup is merged.
I think the order should be:

1. Merge dev into all non-trivial branches.
2. astyle dev
3. Optionally, astyle these branches. This may or may not be necessary.
4. Merge dev again into the same branches as above.

[Desmis]

I approve @Beep6581 because it seems to me a reasonable solution and as I'm messy, my code would be totally unreadable without formatting, but if it is necessary to change formatting tool, no problem :)

[Floessie]

We don't have to astyle everything again. The question is rather: Is everybody content with the results if we ditch pad-first-paren-out and welcome unpad-paren? Or is it better to leave out unpad-paren?

[agriggio]

no space after function name is my preference, but space after control-flow structures. e.g.:

if (cond) {
    call_f(argument);
}

since you asked :-)

[heckflosse]

👍 for ditching pad-first-paren-out and using unpad-paren

[Hombre57]

I prefer having a clear coding style done by astyle or any other "style sanitizer", whatever you prefer (but Eclipse and me prefer not having space between function name and parenthesis).

Please also note that you can disable astyle locally by setting a // *INDENT-OFF* and // *INDENT-ON* comment (on single line). It's used in batchtoolpanelcoord.cc lin 218 to keep code compact.

As to applying astyle to the entire code, give me a solution for non-merged branch first, I really wouln't want to deal with thousands of conflicts.

[Beep6581]

@Hombre57 my suggestion should prevent merge conflicts. We can put this to the test once procparams-cleanup is merged.

[Floessie]

So, I changed my rawtherapee.astyle and applied it to procparams.*. I like the outcome in general, but astyle completely messes brace inits about:

@@ -350,20 +354,20 @@ ToneCurveParams::ToneCurveParams() :
     method("Blend"),
     expcomp(0),
     curve{
-        DCT_Linear
-    },
-    curve2{
-        DCT_Linear
-    },
-    curveMode(ToneCurveParams::TcMode::STD),
-    curveMode2(ToneCurveParams::TcMode::STD),
-    brightness(0),
-    black(0),
-    contrast(0),
-    saturation(0),
-    shcompr(50),
-    hlcompr(0),
-    hlcomprthresh(33)
+    DCT_Linear
+},
+curve2{
+    DCT_Linear
+},
+curveMode(ToneCurveParams::TcMode::STD),
+          curveMode2(ToneCurveParams::TcMode::STD),
+          brightness(0),
+          black(0),
+          contrast(0),
+          saturation(0),
+          shcompr(50),
+          hlcompr(0),
+          hlcomprthresh(33)
 {
 }

So, @Beep6581, I wouldn't run astyle through all files, as it will definitly do harm to those places. I'd suggest a more common-sense approach: Whenever we work on a file and see there are indentation or spacing problems, we first run it through astyle and fix the (hopefully few) places astyle goofed up. This guarantees a personally reviewed process with fewer hickups and surprises than a fully automated approach. Adding exceptions for astyle here and there as @Hombre57 mentioned won't make the code cleaner, IMHO.

Best,
Flössie

[Beep6581]

Issue confirmed, ticket opened: https://sourceforge.net/p/astyle/bugs/467/

Agreed that exceptions are to be avoided.

Commits and merges which format code should be separate from commits and merges which make code changes.