Apply clang-format to whole codebase

[ThisIsFineTM]

The formatting is a little inconsistent from file to file. We have a clang-format config present in the repo already: https://github.com/openzim/zim-tools/blob/main/.clang-format

Could we run clang-format on everything? I can also add precommit hooks in contrib that developers could opt-in to for automatically running clang-format.

[kelson42]

@veloman-yunkan Do we want that or would we prefer to adopt an other norm?

[veloman-yunkan]

I have no preference on this. But whichever policy/standard we adopt, I think it should be the same across all projects in openzim (and probably kiwix as well).

[ThisIsFineTM]

It looks like zim-tools, libkiwix, and kiwix-tools are using the same .clang-format file.
https://github.com/kiwix/libkiwix/blob/main/.clang-format
https://github.com/kiwix/kiwix-tools/blob/main/.clang-format

libzim doesn't have one.

[kelson42]

@ThisIsFineTM @veloman-yunkan what would be reference clang format file? We need to have a proposal and make a decision.

[mgautierfr]

Thoses clang format files have been created by myself years (8) ago.
They are all the same so I would say that we just have to use it.

If someone disagree with the formating, we can open an issue to discuss about it and modify the format (or not).

[ThisIsFineTM]

Using what is there already is probably fine as long as its applied consistently. If you want to experiment with the different clang-format settings, this has an interactive view you can play with:
https://clang-format-configurator.site

[veloman-yunkan]

@ThisIsFineTM @veloman-yunkan what would be reference clang format file? We need to have a proposal and make a decision.

I concur with @mgautierfr, if we decide to introduce standardized formatting let's just use whatever .clang-format is already there.

[ThisIsFineTM]

I should not be the one to submit the PR on this since it will touch so much of the code base. It would be more appropriate for one of the maintainers to do it. For convenience, though:

find -type f -name "*.h" -o -name "*.cpp" -exec clang-format -i {} \;

As a semi-relevant side note, I've personally used the MIT Licence'd clang-format precommit hooks from https://github.com/barisione/clang-format-hooks.git and they've worked nicely.

[kelson42]

@ThisIsFineTM Code maintainers agree to go we current clang format configuration for all our cpp repositories.

What I expect to close this issue is:

• Intoduce a formal format checking job in CI
• fix the code

Considering that this only formatting changes, this is fine to me if you do it. Actually, I encourage you to do it.

Once properly done here, I will then do the necessary to trigger such an effort in all our other cpp repositories.

Thank you again very much for pushing this!

[ThisIsFineTM]

@kelson42 I'll dig around github-ci/actions for the common guidance for setting that up. There are several entries in the marketplace which end up just running clang-format through a docker container. Do you have any preference on the implementation or just "get something that works that works with our license"?

[ThisIsFineTM]

@mgautierfr @veloman-yunkan @kelson42

I've gone through and applied the clang-format that exists in the repo to the codebase locally and noticed a few things in the diff that got changed which were probably better left as they were, or with an equivalent setting in clang-format. I'll number the examples to make it easier to discuss. Sorry that I didn't do this earlier. If these are fine I'll incorporate them into the CI format checking work. The tentative plan is to spin up an Alpine Linux 3.21 docker container with clang-format19 installed through the clang19-extra-tools package.

For reference, clang-format can be turned off in a code block with:

// clang-format off
...
// clang-format on

0. Standard: Cpp17
   Making consistent with the current build flag.

1. Add option AlignArrayOfStructures: Left
   This flag is more consistent with how the code is already formatted. eg.:
   

   zim-tools/src/tools.cpp

   Line 483 in b4a1fa0

   static const std::map<std::string, UriKind> uriSchemes = {

   
   current

    static const std::map<std::string, UriKind> uriSchemes = {
        { "javascript", UriKind::JAVASCRIPT },
        { "mailto",     UriKind::MAILTO     },
        { "tel",        UriKind::TEL        },
        { "sip",        UriKind::SIP        },
        { "geo",        UriKind::GEO        },
        { "data",       UriKind::DATA       },
        { "xmpp",       UriKind::XMPP       },
        { "news",       UriKind::NEWS       },
        { "urn",        UriKind::URN        }
    };

existing clang-format config applied

  static const std::map<std::string, UriKind> uriSchemes
      = {{"javascript", UriKind::JAVASCRIPT},
         {"mailto", UriKind::MAILTO},
         {"tel", UriKind::TEL},
         {"sip", UriKind::SIP},
         {"geo", UriKind::GEO},
         {"data", UriKind::DATA},
         {"xmpp", UriKind::XMPP},
         {"news", UriKind::NEWS},
         {"urn", UriKind::URN}};

AlignArrayOfStructures: Left

  static const std::map<std::string, UriKind> uriSchemes = {
      {"javascript", UriKind::JAVASCRIPT},
      {"mailto",     UriKind::MAILTO    },
      {"tel",        UriKind::TEL       },
      {"sip",        UriKind::SIP       },
      {"geo",        UriKind::GEO       },
      {"data",       UriKind::DATA      },
      {"xmpp",       UriKind::XMPP      },
      {"news",       UriKind::NEWS      },
      {"urn",        UriKind::URN       }
  };

2. InsertBraces: true
   This option adds braces to one-liner control flow statements that don't already have it.

3. Add options BreakConstructorInitializers: BeforeComma and PackConstructorInitializers: Never
   More consistent with how several constructor initializers are already written.
   

   zim-tools/src/zimcheck/checks.cpp

   Line 297 in b4a1fa0

   ArticleChecker(const zim::Archive& _archive, ErrorLogger& _reporter, ProgressBar& _progress, EnabledTests _checks)

   
   current

    ArticleChecker(const zim::Archive& _archive, ErrorLogger& _reporter, ProgressBar& _progress, EnabledTests _checks)
        : archive(_archive)
        , reporter(_reporter)
        , progress(_progress)
        , checks(_checks)
        , linkStatusCache(64*1024)

existing clang-format config applied

  ArticleChecker(const zim::Archive& _archive,
                 ErrorLogger& _reporter,
                 ProgressBar& _progress,
                 EnabledTests _checks)
      : archive(_archive),
        reporter(_reporter),
        progress(_progress),
        checks(_checks),
        linkStatusCache(64 * 1024)

BreakConstructorInitializers: BeforeComma and PackConstructorInitializers Never applied

  ArticleChecker(const zim::Archive& _archive,
                 ErrorLogger& _reporter,
                 ProgressBar& _progress,
                 EnabledTests _checks)
      : archive(_archive)
      , reporter(_reporter)
      , progress(_progress)
      , checks(_checks)
      , linkStatusCache(64 * 1024)