Skip to content

clang-format issues settings.cpp #2587

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
pgScorpio wants to merge 2 commits into from
Closed

clang-format issues settings.cpp #2587

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
79a6674
Changed clang-format off/on locations
pgScorpio Apr 8, 2022
0573a9c
Rework (forgot to remove a clang-format on)
pgScorpio Apr 8, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
76 changes: 40 additions & 36 deletions src/settings.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -458,14 +458,14 @@ void CClientSettings::ReadSettingsFromXML ( const QDomDocument& IniXMLDocument,
// custom directories
// clang-format off
// TODO compatibility to old version (< 3.6.1)
QString strDirectoryAddress = GetIniSetting ( IniXMLDocument, "client", "centralservaddr", "" );
// clang-format on
QString strDirectoryAddress = GetIniSetting ( IniXMLDocument, "client", "centralservaddr", "" );
for ( iIdx = 0; iIdx < MAX_NUM_SERVER_ADDR_ITEMS; iIdx++ )
{
// clang-format off
Comment on lines 461 to 465
Copy link
Member

@hoffie hoffie Apr 8, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

Copy link
Contributor Author

@pgScorpio pgScorpio Apr 8, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

@hoffie hoffie Apr 8, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

@pgScorpio pgScorpio Apr 8, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

// TODO compatibility to old version (< 3.8.2)
strDirectoryAddress = GetIniSetting ( IniXMLDocument, "client", QString ( "centralservaddr%1" ).arg ( iIdx ), strDirectoryAddress );
// clang-format on
strDirectoryAddress = GetIniSetting ( IniXMLDocument, "client", QString ( "centralservaddr%1" ).arg ( iIdx ), strDirectoryAddress );
vstrDirectoryAddress[iIdx] = GetIniSetting ( IniXMLDocument, "client", QString ( "directoryaddress%1" ).arg ( iIdx ), strDirectoryAddress );
strDirectoryAddress = "";
}
Expand All @@ -474,16 +474,18 @@ strDirectoryAddress = GetIniSetting ( IniXMLDocument, "client", QString ( "centr
// clang-format off
// TODO compatibility to old version (<3.4.7)
// only the case that "centralservaddr" was set in old ini must be considered
if ( !vstrDirectoryAddress[0].isEmpty() && GetFlagIniSet ( IniXMLDocument, "client", "defcentservaddr", bValue ) && !bValue )
{
eDirectoryType = AT_CUSTOM;
}
// clang-format on
if ( !vstrDirectoryAddress[0].isEmpty() && GetFlagIniSet ( IniXMLDocument, "client", "defcentservaddr", bValue ) && !bValue )
{
eDirectoryType = AT_CUSTOM;
}
// clang-format off
// TODO compatibility to old version (< 3.8.2)
else if ( GetNumericIniSet ( IniXMLDocument, "client", "centservaddrtype", 0, static_cast<int> ( AT_CUSTOM ), iValue ) )
{
eDirectoryType = static_cast<EDirectoryType> ( iValue );
}
// clang-format on
else if ( GetNumericIniSet ( IniXMLDocument, "client", "centservaddrtype", 0, static_cast<int> ( AT_CUSTOM ), iValue ) )
{
eDirectoryType = static_cast<EDirectoryType> ( iValue );
}
else if ( GetNumericIniSet ( IniXMLDocument, "client", "directorytype", 0, static_cast<int> ( AT_CUSTOM ), iValue ) )
{
eDirectoryType = static_cast<EDirectoryType> ( iValue );
Expand Down Expand Up @@ -822,8 +824,8 @@ void CServerSettings::ReadSettingsFromXML ( const QDomDocument& IniXMLDocument,
QString directoryAddress = "";
// clang-format off
// TODO compatibility to old version < 3.8.2
directoryAddress = GetIniSetting ( IniXMLDocument, "server", "centralservaddr", directoryAddress );
// clang-format on
directoryAddress = GetIniSetting ( IniXMLDocument, "server", "centralservaddr", directoryAddress );
directoryAddress = GetIniSetting ( IniXMLDocument, "server", "directoryaddress", directoryAddress );

pServer->SetDirectoryAddress ( directoryAddress );
Expand All @@ -843,40 +845,42 @@ directoryAddress = GetIniSetting ( IniXMLDocument, "server", "centralservaddr",
{
// clang-format off
// TODO compatibility to old version < 3.4.7
if ( GetFlagIniSet ( IniXMLDocument, "server", "defcentservaddr", bValue ) )
{
directoryType = bValue ? AT_DEFAULT : AT_CUSTOM;
}
else
// clang-format on

// 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
Copy link
Contributor Author

@pgScorpio pgScorpio Apr 8, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

if ( GetFlagIniSet ( IniXMLDocument, "server", "defcentservaddr", bValue ) )
{
directoryType = bValue ? AT_DEFAULT : AT_CUSTOM;
}
// clang-format off
// TODO compatibility to old version < 3.8.2
if ( GetNumericIniSet ( IniXMLDocument, "server", "centservaddrtype", static_cast<int> ( AT_DEFAULT ), static_cast<int> ( AT_CUSTOM ), iValue ) )
{
directoryType = static_cast<EDirectoryType> ( iValue );
}
else
// clang-format on
if ( GetNumericIniSet ( IniXMLDocument,
"server",
"directorytype",
static_cast<int> ( AT_NONE ),
static_cast<int> ( AT_CUSTOM ),
iValue ) )
// if "directorytype" itself is set, use it (note "AT_NONE", "AT_DEFAULT" and "AT_CUSTOM" are min/max directory type here)
// clang-format on
else if ( GetNumericIniSet ( IniXMLDocument,
"server",
"centservaddrtype",
static_cast<int> ( AT_DEFAULT ),
static_cast<int> ( AT_CUSTOM ),
iValue ) )
{
directoryType = static_cast<EDirectoryType> ( iValue );
}
else if ( GetNumericIniSet ( IniXMLDocument,
"server",
"directorytype",
static_cast<int> ( AT_NONE ),
static_cast<int> ( AT_CUSTOM ),
iValue ) )
{
directoryType = static_cast<EDirectoryType> ( iValue );
}

// clang-format off
// TODO compatibility to old version < 3.9.0
// override type to AT_NONE if servlistenabled exists and is false
if ( GetFlagIniSet ( IniXMLDocument, "server", "servlistenabled", bValue ) && !bValue )
{
directoryType = AT_NONE;
}
// clang-format on
if ( GetFlagIniSet ( IniXMLDocument, "server", "servlistenabled", bValue ) && !bValue )
{
directoryType = AT_NONE;
}
}

pServer->SetDirectoryType ( directoryType );
Expand Down