clang-format issues settings.cpp

[pgScorpio]

Fixing clang-format issues caused by different behaviour of clang-format
v10 vs clang-format v13.

clang-format v10 has issues when declarations/statements are partly inside a clang-format off block.

So opening and closing brackets should always be neither or both inside the block.
Also compount statements like 'else if' should never be broken by a clang-format off/on.

To avoid these clang-format version issues at all it's the safest to use clang-format on/off
only around comments or complete declarations/definitions.

Short description of changes
Moved clang-format off/on locations

CHANGELOG:

Context: Fixes an issue?

It does not fix the issue, but it's a work-around for the clang-format version problem

Does this change need documentation? What needs to be documented and how?
Maybe we should document not to use clang-format off/on inside declarations/definitions, unless it's only around a comment ?

Status of this Pull Request

What is missing until this pull request can be merged?

Checklist

• I've verified that this Pull Request follows the general code principles
• I tested my code and it does what I want
• My code follows the style guide
• I waited some time after this Pull Request was opened and all GitHub checks completed without errors.
• I've filled all the content above

[hoffie]

I think the whole point of "out-denting" version-specific parts is to make it more visibile.
This PR mostly reverses this except for the comment, so I don't see the benefits...?

Just to mention again, I don't like this style at all, but it is what it is -- we should aim for consistency here one way or the other.

Maybe there are alternative approaches to this altogether, which do not involve indentation hacks?
(cc @softins @pljones)

For example, what about using #if for those code parts? That would serve as a standardized reminder, would visually stand out and could easily be switched off/removed when the time has come.

E.g.
global.h:

#define COMPAT_JAMULUS_BEFORE_3_6_1 1

Edit: As pointed out by @pgScorpio below, the following example covers the part of the code (but still showcases the idea).

Suggested change

	// clang-format on
	QString strDirectoryAddress = GetIniSetting ( IniXMLDocument, "client", "centralservaddr", "" );
	for ( iIdx = 0; iIdx < MAX_NUM_SERVER_ADDR_ITEMS; iIdx++ )
	{
	// clang-format off
	#if COMPAT_JAMULUS_BEFORE_3_6_1
	QString strDirectoryAddress = GetIniSetting ( IniXMLDocument, "client", "centralservaddr", "" );
	for ( iIdx = 0; iIdx < MAX_NUM_SERVER_ADDR_ITEMS; iIdx++ )
	{
	#endif

This might accumulate multiple such #defines, but it would also serve as a central place to disable those for testing or to know what to remove at some point.

(Note: This is just an idea. Before doing anything in this direction, this needs agreement)

[pgScorpio]

This PR mostly reverses this except for the comment, so I don't see the benefits...?

As said in the commit message clang-format v10 and clang-format v13 behave different when disabling clang format for parts of a declaration/definition. So when locally clang-formatted with v13 and checked with v10 the check fails.
So this PR changes it to "only the comments" since then the clang-format versions behave the same and the test will pass. So this is a great benefit for clang-format v13 users....

For example, what about using #if for those code parts? That would serve as a standardized reminder, would visually stand out and could easily be switched off/removed when the time has come.

I like that solution for the "compatibility issues" too. Maybe you can open a new issue on that.

But this PR is specifically to by-pass the current clang-format issues with settings.cpp

PS: Your #if example won't compile, for the same reason that clang-format v10 has problems. Excluding partial definitions (you replaced a clang-format on / clang-format off instead of a clang-format off / clang-format on) ;-)

[hoffie]

So this PR changes it to "only the comments" since then the clang-format versions behave the same and the test will pass. So this is a great benefit for clang-format v13 users....

Yes, I'm aware of the issue, but I don't think we should modify the code this way (and inconsistently so) to work around glitches in the tools we use.

For clang-format, we should accept what v10 does for now because that's what has been agreed on. Of course, the situation should be fixed and improved and I'm hoping for #2559 here.

As for the coding style in question, I'd really like to continue thinking about the #define-based proposal above. You're right that this should be done in a separate issue though. Edit: I've just opened #2591for this.

PS: Your #if example won't compile [...] (you replaced a clang-format on / clang-format off instead of a clang-format off / clang-format on) ;-)

Umm, yes, sorry, but I think it was still able to showcase the idea. :)

[pgScorpio]

Umm, yes, sorry, but I think it was still able to showcase the idea. :)

Yes, the idea was still very clear, and I agree that would be a better solution for the compatibility code.

[pgScorpio]

Oooops, I forgot to remove this clang-format on...

And the check failed again... :-((
But this proves how critical the placing of the clang-format off/on is in v10

[pgScorpio]

Issue solved by PR #2600