RFC: Add clang-format style for project

[AndreasFuchsTPM]

I would like to make the complete codebase of this project consistent in terms of style.

Thus I hereby propose our new coding style. You can check the result by grabbing the .clang-format file and running

find src/ test/ include/ -type f \( -name *.c -or -name *.h \) | xargs clang-format -i

and comment on the resulting git diff...

If everyone is fine, I'd like to apply the complete changes to the code base and then also add a CI job that checks for this style. Also adding a mention in "CONTRIBUTING.md".

What do y'all think ?

Edit: I added a MacroBlockBegin statement to account for "statecase" in FAPI. This will now indent all new states, but this actually makes it weirdly readable...

[joholl]

This is great! Just a question: is the goal to establish a new coding style or to match the old one as best as possible (both would be fine by me)?

We might also want to add .git-blame-ignore-revs which makes GitHub ignore formatting commits in the blame view. One can also pass this file to git blame --ignore-revs-file or even set it via git config --local blame.ignoreRevsFile.

We probably want to revise (or remove) our coding_standard_c.md, too.

[JuergenReppSIT]

@AndreasFuchsTPM On Ubuntu 22.04 I got the following error ?

find src/ test/ include/ -type f \( -name *.c -or -name *.h \) | xargs clang-format -i
/workspace/tpm2-tss/.clang-format:14:3: error: unknown enumerated scalar
  Enabled: true
  ^

[wxleong]

@JuergenReppSIT You will need clang-format version 15 to support that (use apt install clang-format-15).

@AndreasFuchsTPM Perhaps consider adding ReflowComments: true as well. *Ignore this; ReflowComments is already part of the GNU style.

[joholl]

Our fapi statecase(...) macros are broken because clang-format thinks that they are functions. With clang-format v17 they ship the Macros config which let's us tell clang-format that those evaluate to case statements:

Macros:
- statecase(VAR, STATE)=case 0:;
- general_failure(VAR)=default:;
- statecasedefault(VAR)=default:;
- statecasedefault_error(VAR, r, label)=default:;

To try this out (even on Ubuntu), you can use a dockerized clang-format:

docker run -u 1000 -v $PWD:$PWD xianpengshen/clang-tools:17 clang-format -i $PWD/**/*.c $PWD/**/*.h

This results in the following formatting. What I could not get working is formatting comments after the case correctly (but I would be able to live with the result):

    switch (context->io_state) {
    statecase(context->io_state, IO_INIT)
    /* Prepare the loading of the object. */
        r = ifapi_keystore_load_async(&context->keystore, &context->io, path);
        return_if_error2(r, "Could not open: %s", path);
        fallthrough;

Depending on our preferences, we can set IndentCaseLabels to true or false (default for GNU style). False seems to result in less changes.

To measure the changes:

git diff --shortstat | grep -oP '(?<=changed, )\d*'

[joholl]

I'd like to request a change when it comes to function declarations/definitions. My suggestion would change a lot of lines of code, but is IMO most in line with what we do currently in the fapi.

The following applies to declarations and definitions equally.

Currently

# AlignAfterOpenBracket: Align  # default Align
# AlignConsecutiveDeclarations: false  # default false
# BinPackParameters: true  # default true
# AllowAllParametersOfDeclarationOnNextLine: true  # default true

TSS2_RC Fapi_CreateSeal_Async(FAPI_CONTEXT *context, char const *path, char const *type,
                              size_t size, char const *policyPath, char const *authValue,
                              uint8_t const *data);

Better:

AlignAfterOpenBracket: AlwaysBreak  # default Align
AlignConsecutiveDeclarations: true
BinPackParameters: false
AllowAllParametersOfDeclarationOnNextLine: false

TSS2_RC Fapi_CreateSeal_Async(
    FAPI_CONTEXT  *context,
    char const    *path,
    char const    *type,
    size_t         size,
    char const    *policyPath,
    char const    *authValue,
    uint8_t const *data);

[JuergenReppSIT]

The statecase macro now looks better with @johol's suggestion e.g.:

  statecase(context->state, PROVISION_READ_HIERARCHY)
         ;
         path = command->pathlist[command->path_idx];

Only superfluous semicolons are moved to the next line. But I get compile errors e.g.:

src/tss2-fapi/ifapi_eventlog_system.h:39:5: error: unknown type name ‘UINT8_ARY’
   39 |     UINT8_ARY data;
      |     ^~~~~~~~~

@joholl Can you compile tss after executing clang-format-17?

[joholl]

@JuergenReppSIT I also get these compile errors. That is because clang-format sorts the includes alphabetically and we do not enforce include correctness (see also #2666).

Btw: cmocka assumes four includes, so we would also need to add something like this.

/* the following includes are needed by cmocka.h */
// clang-format off
#include <stdarg.h>
#include <stddef.h>
#include <stdint.h>
#include <setjmp.h>

#include <cmocka.h>
// clang-format on

Let me try to create a commit for that.

[AndreasFuchsTPM]

Let me try to create a commit for that.

You might wanna wait for https://github.com/tpm2-software/tpm2-tss/pull/2823

[wxleong]

@JuergenReppSIT @joholl

Only superfluous semicolons are moved to the next line.

It looks like this issue is caused by the appended semicolon when these macros are used.

There are code with the semicolon:

    switch (context->flush_object_state) {
    statecase(context->flush_object_state, FLUSH_INIT);

Also code that doesnt:

    switch (context->io_state) {
    statecase(context->io_state, IO_INIT)

Perhaps we should clean up the semicolons following these macros in the code for consistency.

[joholl]

@JuergenReppSIT @wxleong

The semicolon issue is fixed with the following in .clang-format. Then you can have a semicolon or not, both works. (The comments will still have a weird identation, I could not fix that).

Macros:
- statecase(VAR, STATE)=case 0:fn()
- general_failure(VAR)=default:fn()
- statecasedefault(VAR)=default:fn()
- statecasedefault_error(VAR, r, label)=default:fn()

[wxleong]

@joholl Actually, that will trigger another issue:

TSS2_RC
ifapi_get_key_public(const char *path, TPMT_PUBLIC *public, void *ctx) {
    TSS2_RC r = TSS2_RC_SUCCESS;
    IFAPI_OBJECT object;
    FAPI_CONTEXT *context = ctx;

    switch (context->io_state) {
    statecase(context->io_state, IO_INIT)
    /* Prepare the loading of the object. */ r = ifapi_keystore_load_async(&context->keystore, &context->io, path);
        return_if_error2(r, "Could not open: %s", path);
        fallthrough;

It seems like this resolves the issue where the line extends behind the comment:

Macros:
- statecase(VAR, STATE)=case 0:{}
- general_failure(VAR)=default:{}
- statecasedefault(VAR)=default:{}
- statecasedefault_error(VAR, r, label)=default:{}

[AndreasFuchsTPM]

I did add the things we talked about. Slight difference is to keep AlignAfterOpenBracket: Align Let me know what you think...

You can use this for testing (thanks to johol):

docker run -u 1000 -v $PWD:$PWD xianpengshen/clang-tools:17 clang-format -i $(find -name '*.c' | xargs realpath)
docker run -u 1000 -v $PWD:$PWD xianpengshen/clang-tools:17 clang-format -i $(find -name '*.h' | xargs realpath)

If y'all are ok, I will go ahead, let it run and then check it in using some smaller chunks...