Minimal updated subset of changes from #2017 (clang-format) #2977
Conversation
Jakuje
changed the title
|
I noticed that we currently have `TabWidth 4' defined. I don't have a problem with this setting, but I'll just remind you of the years-old debate about TAB in the Linux kernel. https://public-inbox.org/git/[email protected]/ probably the most explanatory email from the mentioned debate: |
Oh, The editorconfig has tab width 8 and clang 4. This is clearly an inconsistency I wanted to catch before merging this. But it should not have any effect in the resulting formatting itself.
The indentation debates will never be over. I read through this mail, but it really did does not bring much insights to what is the reason and what are advantages. There was already a lot of discussion in the #2017. I was used to tab size 4 most of the time, but I agree that tab width 8 have some advantages such as avoiding too much indentation or better readability, especially if we use spaces for continuation indentation. So I do not have strong opinion. While trying to reformat some code with current configuration, there are still things I do not like I mentioned in the previous PR, so I will try again here if there will be some other insights:
CK_ATTRIBUTE template[] = {
- { CKA_CLASS, &class, sizeof(class) },
- { CKA_KEY_TYPE, &keyType, sizeof(keyType) },
- { CKA_LABEL, label, sizeof(label)-1 },
- { CKA_ENCRYPT, &_true, sizeof(_true) },
- { CKA_DECRYPT, &_true, sizeof(_true) },
- { CKA_VALUE, key, keysize },
- { CKA_VALUE_LEN, &keylen, sizeof(keylen) },
+ {CKA_CLASS, &class, sizeof(class) },
+ {CKA_KEY_TYPE, &keyType, sizeof(keyType) },
+ {CKA_LABEL, label, sizeof(label) - 1},
+ {CKA_ENCRYPT, &_true, sizeof(_true) },
+ {CKA_DECRYPT, &_true, sizeof(_true) },
+ {CKA_VALUE, key, keysize },
+ {CKA_VALUE_LEN, &keylen, sizeof(keylen) },
};
if (!find_object(session, CKO_PUBLIC_KEY, &hWrappingKey,
- opt_object_id_len ? opt_object_id : NULL, opt_object_id_len, 0))
+ opt_object_id_len ? opt_object_id : NULL, opt_object_id_len, 0))
if (!find_object(session, CKO_SECRET_KEY, &hWrappingKey,
- opt_object_id_len ? opt_object_id : NULL, opt_object_id_len, 0))
+ opt_object_id_len ? opt_object_id : NULL, opt_object_id_len, 0))
util_fatal("Public/secret key (wrapping key) not found");
} else if (!find_object(session, CKO_SECRET_KEY, &object,
- opt_object_id_len ? opt_object_id : NULL, opt_object_id_len, 0))
+ opt_object_id_len ? opt_object_id : NULL, opt_object_id_len, 0))
util_fatal("Secret key not found");
util_fatal("Unknown key pair type %s, valid key types for mechanism GOSTR3410 are GOSTR3410-2001:{A,B,C},"
- " GOSTR3410-2012-256:{A,B,C,D}, GOSTR3410-2012-512:{A,B,C}", type);
+ " GOSTR3410-2012-256:{A,B,C,D}, GOSTR3410-2012-512:{A,B,C}",
+ type);
(now it tries to reformat also the json files with p11test results, but thats something we can handle) |
This allows for voluntary formatting with clang-format (or IDE tools like clang-format plugin for VS Code). Once a suitable formatting configuration is found, automatic formatting can be configured Tweaked by Jakub Jelen
Usage: git config blame.ignoreRevsFile .ignoreRevsFile
|
I think most of the time we avoided a mixture of spaces and tabs by replacing 8 spaces with a tab so that IndentWidth and TabWidth should both be
I think ContinuationIndentWidth should be
Your examples above originally contain a mixture of some tabs and a single leading space. That single space is superfluous and may confuse clang-format. could you please try removing this space first and then apply clang-format?
That particular example looks strange, indeed. |
My reading of the documentation is the value as used in the clang-format is not additional indent, but the whole indent so we need 16 if we want to use two tabs for continuation: https://clang.llvm.org/docs/ClangFormatStyleOptions.html#continuationindentwidth The problem is that, I think, also the struct initialization is considered a continuation here. It should be solved with
It did not help. I think it tries to align something somewhere, but I do not see what and where. I tried also different values for } else if (!find_object(session, CKO_PRIVATE_KEY, &object,
- opt_object_id_len ? opt_object_id : NULL, opt_object_id_len, 0))
+ opt_object_id_len ? opt_object_id : NULL, opt_object_id_len, 0))
if (!find_object(session, CKO_SECRET_KEY, &object,
- opt_object_id_len ? opt_object_id : NULL, opt_object_id_len, 0))
+ opt_object_id_len ? opt_object_id : NULL, opt_object_id_len, 0))
util_fatal("Private/secret key not found");
I though we could get around this with |
|
I don't see an easy way out of the formatting problems of clang-format. What about adding the configuration files as suggested right now, but allowing the workflow to fail? |
I am ok with that. Maybe adding also a note to the pipeline that its not mandatory to follow all the suggestions. Unfortunately, github does not have any way to make the result warning as githlab does so there is only a way to let it fail (drawing attention to that by PR contributor) or ignore the results altogether, making the results technically invisible for anyone searching for that. From this, I believe the first option (the current state in this PR) is more reasonable. |
|
If I'm not mistaken, than a mandatory formatter workflow that doesn't work reliably will constantly show our project as "not building" on the main page. To avoid that, a succeeding job with somewhat invisible results should be better. Since we review all pull request, we can bring up the formatting issues by pointing to the results of this job if needed. |
If you mean the badges, in the readme, they are tied to the particular workflow file (now linux, osx, ...). This is separate workflow file that does not have to show up on the main page. And actually it does not even have to run on the branches/pushes, but only on the PRs as proposed right now. |
Based on last comment in #2017 from @popovec I pulled the .clang-format and some minimal changes to the separate branch, that can be merged if we will agree on that format.
I played with it a bit more, but I am still not completely satisfied with the current configuration as it sometimes formats some complicated constructs in non-intuitive way. But it is something we can try to tune later (or rewrite the code to avoid these constructs).
In any case, review here just to make sure the .clang-format, .editorconfig and description of the format in contributing.md matches would be welcomed. Similarly if we should add some more examples or some look odd.
Checklist