clang-format issues settings.cpp#2587
clang-format issues settings.cpp#2587pgScorpio wants to merge 2 commits intojamulussoftware:masterfrom
Conversation
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.
| // clang-format on | ||
| QString strDirectoryAddress = GetIniSetting ( IniXMLDocument, "client", "centralservaddr", "" ); | ||
| for ( iIdx = 0; iIdx < MAX_NUM_SERVER_ADDR_ITEMS; iIdx++ ) | ||
| { | ||
| // clang-format off |
There was a problem hiding this comment.
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).
| // 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)
There was a problem hiding this comment.
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) ;-)
There was a problem hiding this comment.
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. :)
There was a problem hiding this comment.
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.
|
|
||
| // if "directorytype" itself is set, use it (note "AT_NONE", "AT_DEFAULT" and "AT_CUSTOM" are min/max directory type here) | ||
| // clang-format off | ||
| // clang-format on |
There was a problem hiding this comment.
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
|
Issue solved by PR #2600 |
2 participants
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