Code formatting

[xezon]

It was not clear to me from the meeting what we now do with the code formatting. We did agree that changing all formatting can make integrations more difficult. Some points:

Manual format

CRLF

Line endings are \r\n. Are we ok with this? Is Linux ok with this? Generally, my understanding is that one settles on \n. Do we want to change it for all files. Or not?

Tabs vs Spaces

The code uses 2 space characters or tabs as 2 space character equivalent to do indentation. It is not possible to comfortably switch to 4 space character wide tabs. How about we apply a format pass to make all whitespacing consistent? Leading tabs or leading spaces everywhere, no tabs after code words.

Whitespace difference across forks should cause no merge conflicts, because merge tools should have the ability to ignore whitespace differences on merge.

Sample image from P4Merge

Format for new code written

If we do not apply code formatting on the original code, how about we at least try to apply formatting on all new code? How can we do this?

One way to do it for Visual Studio is the editorconfig. It is unclear however how well this will work in practice. https://learn.microsoft.com/en-us/visualstudio/ide/create-portable-custom-editor-options?view=vs-

Disadvantages of manual formatting

• Code may look different across files and functions
• Lines can become longer than what github, merge tools will comfortably show on one page, requires scrolling

Advantages of manual formatting

• Easy submissions
• Easily mergable with other forks and all contributors
• No frustrations from bad auto formatting

Clang Format

Or do we want to apply clang formatting, closely matching the original code style? It will incur merge conflicts, unless the author applies clang format with our formatting rules before attempting the merge. Does it work? Is this practical? Do we need a guide?

Disadvantage of clang format

• Makes submissions more complicated, error prone when enforced in github builds
• Requires user to install and use exact clang format version as is used in github builds
• clang format can be absurdly frustrating when changing the code in stupid ways the programmer does not want to see his code

Advantages of clang format

• All code looks uniformly formatted and is in view

[DevGeniusCode]

My two cents, using Clang will allow for enforcement of formatting rules across a variety of different IDEs, and I also assume that by working on VC22 where we no longer worry about compatibility with 1.04, we will be able to apply design rules to the original current code as well.

[OmniBlade]

Regarding line endings CRLF or LF, don't worry about it. Git by default converts to CRLF on checkout on windows and converts to LF on push so the repo is almost certainly LF already and locally will depend on if you are on Windows or not.

I'm also in favour of automated formatting because ultimately manual formatting will have inconsistencies fall through the cracks. For blocks of code the author wants a particular way, they can use clang-format off/clang-format on comments to book end the code. The downside of it not being consistent between versions of clang-format is very furstrating though as at some point we will need to bump the format version due to deprecation and deal with any slight changes it makes.

[OmniBlade]

I agree to getting them agreed now. Line length in Thyme was set to the max github allows in its viewer which seemed to be a sensible value to standardise on. I'm not wed to the Thyme standards though, I'm happy to change it to whatever.

By default PRs allow a maintainer to push to the PR'd branch unless disabled so if someone has submitted code that isn't formatted correctly and struggled to set it up correctly a maintainer would be able to pull the branch, format it and push it back up.

I think consistency is important and if we have standards its easier to have an automated tool nit pick and prevent a merge rather than maintainers inconsistently nit pick and prevent merges?

[xezon]

In terms of line breaks I would prefer a solution that only puts line breaks at absurd length, not if it is one character over an arbitrary limit.

So for example what frustrates me is a format like this:

int functionWithLongName(int a, int b, int c, int d, int e, int f);
int functionWithAbitLongerNameThanTheOthers(
    int a, int b, int c, int d, int e, int f); // <-- sad
int functionWithAnotherName(int a, int b, int c, int d, int e, int f);
int functionWithAnotherName2(int a, int b, int c, int d, int e, int f);
int functionWithAnotherName3(int a, int b, int c, int d, int e, int f);

The above auto format has made this block of function declarations less readable. It would have been better off this way:

int functionWithLongName(int a, int b, int c, int d, int e, int f);
int functionWithAbitLongerNameThanTheOthers(int a, int b, int c, int d, int e, int f);
int functionWithAnotherName(int a, int b, int c, int d, int e, int f);
int functionWithAnotherName2(int a, int b, int c, int d, int e, int f);
int functionWithAnotherName3(int a, int b, int c, int d, int e, int f);

Basically clang format would only do its format if the presented code is absurd, like:

void   function (int a, int* b, int &c, int )   const
{

  int d   = a* (*b);
if (d <100)
     return d;
else
          return a;

}

But it would not change the format if it looks just fine to a human.

[xezon]

rather than maintainers inconsistently nit pick and prevent merges?

This is a good point. This is likely to happen if programmers come with very exotic format styles from Pluto.

I did agree with the point made by Tarnished Knight that it is perfectly possible to fit into the existing style of a file. Basically the surrounding code is the format law.

Is it perhaps possible to make a minimal format that changes very little of the original and gives enough freedom to craft the code but still enforce some rules, such as indentation, spacings?

For example I think it is perfectly fine to give the programmer the freedom to write a function declaration like so:

void function(
  const ClassTypeA& classNameA,
  const ClassTypeB& classNameB,
  const ClassTypeC& classNameC);

or

void function(const ClassTypeA& classNameA, const ClassTypeB& classNameB, const ClassTypeC& classNameC);

A formatter would force one or the other, and the formatter would not understand which one is easier to read for the human.

[OmniBlade]

I disagree though, I think we need to pick one of those forms and make that the rule for the code base and the prevailing style is the second example. Unless the line runs over some arbitrary length (and I agree it is arbitrary), then we pick a style of declaring for very long function definitions which could be the first example.

The programmer can write it however they want initially and then the formatting pass makes it look how it "should" look from the perspective of the project. Its also worth noting that a project formatting style isn't going to please everyone in every way but I think its important to agree one because if you are going to argue for "programmer creativity" well programmers can get pretty creative and what is and isn't acceptable is again an arbitrary distinction.

[ZenBre4ker]

Yes, please force formatting. Nothing more annoying than having to nitpick formatting and arguing about it.

[xezon]

Can we check with W3DHub what formatting direction they want to go? Perhaps we can sync on it, then collaboration will be better. Can someone link them here? I do not know anyone from W3DHub.

[xezon]

User OWA has seen this and is forwarding it to W3D programmers.

[xezon]

What was the reason for Thyme to use old clang format version 10, when the newest is at version 20 or so?

[ZenBre4ker]

Probably compatibility to vs2015 and older compilers. We do the same at work.... which is quite annoying.

[OmniBlade]

We started formatting years ago and it was probably common to all the ubuntu runners at the time. In hindsight the formatting should only need to be checked by one runner and it probably should be the most up to date one available.

[cfehunter]

As far as W3DHub renegade goes I intend on rolling clang format, but we should align together to ease code sharing.

You can disable clang format for a section of code with a simple // clang-format off // clang-format on. So if you want to format a table for readability or something then you have that option.

As for it making seemingly infuriating changes, well, I personally can't stand having it format as I'm typing. Format on save tends to work fine though. I've been using it personally and professionally for years with no problem.

[xezon]

Hello. I share the infuriation on code format while typing.

I will put up some clang format pain points later that we can hopefully avoid.

[xezon]

My personal clang format pain points

Here are my observations from working with the clang format setup from the Thyme project and its descendants. It may be incomplete.

constructor initializer

Colon and comma ideally preceed the variable initializer on the same line. Otherwise it looks unholy when adding a macro for example. It also means adding a new initializer at the end of the list requires touching 2 lines instead of 1.

bad

BaseHeightMapRenderObjClass::BaseHeightMapRenderObjClass() :
    m_x(0),
    m_y(0),
    m_vertexScorch(nullptr),
    m_indexScorch(nullptr),
    m_scorchTexture(nullptr),
    m_cliffAngle(45.0f)
#if CONDITION
    ,
    m_map(nullptr)
#endif
{
}

better

BaseHeightMapRenderObjClass::BaseHeightMapRenderObjClass()
    : m_x(0)
    , m_y(0)
    , m_vertexScorch(nullptr)
    , m_indexScorch(nullptr)
    , m_scorchTexture(nullptr)
    , m_cliffAngle(45.0f)
#if CONDITION
    , m_map(nullptr)
#endif
{
}

argument packing

bad

Thyme's clang format setup has 3 or 4 argument packing variations for functions declations and function calls, depending on the length of the line. This is not good, because it presents too many kinds of variations.

class MyClass
{
    int Get_Static_Diffuse(int x, int y); // <----- STANDARD
    float Get_Max_Cell_Height(float x, float y) const;
    void Set_Time_Of_Day(TimeOfDayType time);
    void Do_The_Light(VertexFormatXYZDUV2 *vb, // <----- PACKING VARIATION 1
        Vector3 *light,
        Vector3 *normal,
        RefMultiListIterator<RenderObjClass> *lights,
        unsigned char alpha);
    float Get_Height_Map_Height(float x, float y, Coord3D *pos) const;
    void Notify_Shroud_Changed();
    void Add_Tree( // <----- PACKING VARIATION 2
        DrawableID drawable, Coord3D location, float scale, float angle, float random, W3DTreeDrawModuleData const *module);
    void Add_Prop(int id, Coord3D location, float orientation, float scale, const Utf8String &name);
    void Add_Scorch(Vector3 location, float radius, Scorches type);
}

better

Settle on 2 packing variants only, with consistent indentation for arguments.

class MyClass
{
    int Get_Static_Diffuse(int x, int y);
    float Get_Max_Cell_Height(float x, float y) const;
    void Set_Time_Of_Day(TimeOfDayType time);
    void Do_The_Light(
        VertexFormatXYZDUV2 *vb,
        Vector3 *light,
        Vector3 *normal,
        RefMultiListIterator<RenderObjClass> *lights,
        unsigned char alpha);
    float Get_Height_Map_Height(float x, float y, Coord3D *pos) const;
    void Notify_Shroud_Changed();
    void Add_Tree(
        DrawableID drawable,
        Coord3D location,
        float scale,
        float angle,
        float random,
        W3DTreeDrawModuleData const *module);
    void Add_Prop(int id, Coord3D location, float orientation, float scale, const Utf8String &name);
    void Add_Scorch(Vector3 location, float radius, Scorches type);
}

bad

Here is another example of too many packing variations making it harder to look at:

void function()
{
...
    g_thePartitionManager->Reveal_Map_For_Player_Permanently( // <----- PACKING VARIATION 1
        g_thePlayerList->Find_Player_With_NameKey(g_theNameKeyGenerator->Name_To_Key("ReplayObserver"))->Get_Player_Index());
    captainslog_debug("Reveal shroud for %ls whose index is %d", // <----- PACKING VARIATION 2
        g_thePlayerList->Find_Player_With_NameKey(g_theNameKeyGenerator->Name_To_Key("ReplayObserver"))
            ->Get_Player_Display_Name()
            .Str(),
        g_thePlayerList->Find_Player_With_NameKey(g_theNameKeyGenerator->Name_To_Key("ReplayObserver"))->Get_Player_Index());

    if (game_info != nullptr) {
        for (int slot_idx = 0; slot_idx < GameInfo::MAX_SLOTS; slot_idx++) {
            GameSlot *slot = game_info->Get_Slot(slot_idx);

            if (slot != nullptr && slot->Is_Occupied()) {
                Utf8String player_name;
                player_name.Format("player%d", slot_idx);
                Player *player = // <----- UNPLEASANT PACKING
                    g_thePlayerList->Find_Player_With_NameKey(g_theNameKeyGenerator->Name_To_Key(player_name.Str()));

                if (slot->Get_Player_Template() == -2) {
                    captainslog_debug("Clearing shroud for observer %s in playerList slot %d",
                        player_name.Str(),
                        player->Get_Player_Index());
...
}

better

void function()
{
...
    g_thePartitionManager->Reveal_Map_For_Player_Permanently(
        g_thePlayerList->Find_Player_With_NameKey(g_theNameKeyGenerator->Name_To_Key("ReplayObserver"))->Get_Player_Index());
    captainslog_debug(
        "Reveal shroud for %ls whose index is %d",
        g_thePlayerList->Find_Player_With_NameKey(g_theNameKeyGenerator->Name_To_Key("ReplayObserver"))->Get_Player_Display_Name().Str(),
        g_thePlayerList->Find_Player_With_NameKey(g_theNameKeyGenerator->Name_To_Key("ReplayObserver"))->Get_Player_Index());

    if (game_info != nullptr)
    {
        for (int slot_idx = 0; slot_idx < GameInfo::MAX_SLOTS; slot_idx++)
        {
            GameSlot *slot = game_info->Get_Slot(slot_idx);

            if (slot != nullptr && slot->Is_Occupied())
            {
                Utf8String player_name;
                player_name.Format("player%d", slot_idx);
                Player *player = g_thePlayerList->Find_Player_With_NameKey(
                    g_theNameKeyGenerator->Name_To_Key(player_name.Str()));

                if (slot->Get_Player_Template() == -2)
                {
                    captainslog_debug(
                        "Clearing shroud for observer %s in playerList slot %d",
                        player_name.Str(),
                        player->Get_Player_Index());
...
}

indentation spaces

Thyme uses 4 spaces as indentation. I recommend that leading indentation is made with tabs, because this allows the user to configure the tab size in his text editor. Some user may prefer 2 character wide indentation while another user may prefer 4 characters.

Spaces used for indentation

code density

Here the code uses compacted scopes which condense the code. This makes the code more difficult to look at because everything is so tightly packed together. It also greatly mismatches with the formatting of the original EA source code, which presents the code with much more generous spacing, easing the work on the eyes.

bad

    if (g_theNetwork != nullptr) {
        if (g_theLAN != nullptr) {
            captainslog_debug("Starting network game");
            game_info = g_theLAN->Get_My_Game();
            g_theGameInfo = game_info;
        } else {
            captainslog_debug("Starting gamespy game");
            game_info = g_theGameSpyGame;
            g_theGameInfo = g_theGameSpyGame;
        }
    } else if (g_theRecorder != nullptr && g_theRecorder->Get_Mode() == RECORDERMODETYPE_PLAYBACK) {
        game_info = g_theRecorder->Get_Game_Info();
        g_theGameInfo = game_info;
    } else if (m_gameMode == GAME_SKIRMISH) {
        game_info = g_theSkirmishGameInfo;
        g_theGameInfo = g_theSkirmishGameInfo;
    } else if (is_challenge_campaign) {
        game_info = g_theChallengeGameInfo;
        g_theGameInfo = g_theChallengeGameInfo;
    }

better

    if (g_theNetwork != nullptr)
    {
        if (g_theLAN != nullptr)
        {
            captainslog_debug("Starting network game");
            game_info = g_theLAN->Get_My_Game();
            g_theGameInfo = game_info;
        }
        else
        {
            captainslog_debug("Starting gamespy game");
            game_info = g_theGameSpyGame;
            g_theGameInfo = g_theGameSpyGame;
        }
    }
    else if (g_theRecorder != nullptr && g_theRecorder->Get_Mode() == RECORDERMODETYPE_PLAYBACK)
    {
        game_info = g_theRecorder->Get_Game_Info();
        g_theGameInfo = game_info;
    }
    else if (m_gameMode == GAME_SKIRMISH)
    {
        game_info = g_theSkirmishGameInfo;
        g_theGameInfo = g_theSkirmishGameInfo;
    }
    else if (is_challenge_campaign)
    {
        game_info = g_theChallengeGameInfo;
        g_theGameInfo = g_theChallengeGameInfo;
    }

empty function scopes in cpp

clang format may be configured to do this in a cpp:

void W3DModelDraw::React_To_Geometry_Change() {}

void W3DModelDraw::Notify_Draw_Module_Dependency_Cleared() {}

void W3DModelDraw::On_Render_Obj_Recreated() {}

This means if a user types a function body, then plans to write something in but applies formatting by saving the file before that, then the function body {} characters collapse like that. It is not convenient. I suggest that empty functions in cpp always look like this:

void W3DModelDraw::React_To_Geometry_Change()
{
}

void W3DModelDraw::Notify_Draw_Module_Dependency_Cleared()
{
}

void W3DModelDraw::On_Render_Obj_Recreated()
{
}

[xezon]

The point of tabs is that it allows the user to configure a custom indentation width. With spaces this is enforced, therefore giving less flexibility.

[xezon]

I personally prefer the denser code style, the extra space to my eyes looks wasted and redundant and doesn't really impact my ability to read the code.

This is a super power I do not have :-(

[feliwir]

I'm a bit late to the discussion, but i'm definetly in favor of spaces. Having the code look different on any editor you use is not a good thing.
Apart from that the de factor standard for any opensource project (or closed source project) are whitespaces. Regular C++ devs will be quite confused by tabs.

[Fighter19]

I believe it should be mostly consistent.
I'd prefer spaces a bit over tabs given what feliwir was saying.
That being said, it probably would have to be applied project-wide.

[xezon]

Ok. Let's do 4 spaces then.

[cfehunter]

These are fair points.
I took prefer having the initialiser list with the comma as a prefix, for preprocessor switches.

Whitespace wise I would suggest tabs to indent and spaces to align.

I don't really have an opinion on the function stubs.

As for params, I believe we have a few options to control where it puts the braces and line breaks. If you get a function with that many params it should probably be a struct though.

[cfehunter]

I like tabs for the exact reason that the user can configure the depth of indents. I don't really have a strong opinion on it though 😄

…

On Thu, 13 Mar 2025, 09:46 OmniBlade, ***@***.***> wrote: I agree with the empty scopes and initlializers. Can't say I agree with the tab based indentation. In practice it only looks right when indentation is the same as the author intended so why allow it to be altered at all, just make it spaces and it looks consistent for everyone regardless of how their editor is configured. Make it 2, make it 4, but make it spaces. That said, if the formatting is automated it will take care of most stuff anyhow. I personally prefer the denser code style, the extra space to my eyes looks wasted and redundant and doesn't really impact my ability to read the code. However I don't actually care that much and if that is the prevailing style in the current code I can live with it. — Reply to this email directly, view it on GitHub <#409 (reply in thread)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AALJLQLSHRM7D3WYE262BCT2UFHYXAVCNFSM6AAAAABYZ5KKY6VHI2DSMVQWIX3LMV43URDJONRXK43TNFXW4Q3PNVWWK3TUHMYTENBYGU4DSNI> . You are receiving this because you commented.Message ID: <TheSuperHackers/GeneralsGameCode/repo-discussions/409/comments/12485895@ github.com>

[OmniBlade]

But then you won't see the code the way my artistic vision intended :D The auto formatter will make everything conform to whatever we decide at the end of the day, but I dislike reading code intended for some indentation level that isn't my editors default (which mostly is 4 everywhere) and it looking all wrong. With spaces it can never look wrong anywhere because its always the same.

You can't change any other aspect of the formatting, it seems odd to worry about being able to change this particular one which most people never will anyhow?

I'd say the only argument for tabs is that the code already uses them so it will reduce the enventual diff.

[xezon]

Personally I do not feel strongly about leading tabs vs spaces as long as they are 4 spaces, because that is what I am used to. Someone with 2 spaces as tabs may respond differently however :-)

[Kidribeye]

Personalty, I prefer block code solely because of syntax like parenthesis and colons. (◔‿◔)