Skip to content
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.

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

Fix #224: Add a clang-format that matches the best the OpenSSL coding style. #241

Merged
merged 1 commit into from Sep 1, 2023

Conversation

ghost
Copy link

@ghost ghost commented Aug 31, 2023

This commit adds a .clang-format file at the root of the repository. This .clang-format file tries to match the best the OpenSSL coding style.

This commit also reformats the existing code according to that .clang-format.

Finally, it adds a new CircleCI job to ensure that the code is well-formatted.

@ghost ghost requested a review from baentsch as a code owner August 31, 2023 08:29
@ghost ghost marked this pull request as draft August 31, 2023 08:49
@ghost
Copy link
Author

ghost commented Aug 31, 2023

@baentsch , CI jobs are failing because of the generated code that get reformat by clang-format.

Should we add a "post-step" to the code generation job, where we run clang-format?

@baentsch
Copy link
Member

Should we add a "post-step" to the code generation job, where we run clang-format?

Yes -- that would be easier than getting all "jinja2" magic to align (particularly considering #228). I started to look into that when evaluating the impact of #224 but hit some clang version dependency: Is it possible that clang-format is dependent on clang version used? If so I think we also should be explicit in the build env which clang version to use.

@ghost
Copy link
Author

ghost commented Aug 31, 2023

Yes -- that would be easier than getting all "jinja2" magic to align (particularly considering #228).

👍 , will do that.

Is it possible that clang-format is dependent on clang version used? If so I think we also should be explicit in the build env which clang version to use.

It is indeed: some clang-format directives are new and may be unavailable in older versions.
Basically, versions of clang-format follow clang's.

I checked the .clang-format I suggest, and the PPIndentWidth directive is the "latest" one, because it requires clang-13, and LLVM 13 was released in October 2021. Otherwise, the second most "latest" one requires clang-11.

@baentsch
Copy link
Member

because it requires clang-13

I'd be fine mandating that for a build env. The OQS main CI build env is "Ubuntu Jammy" -- and that's at clang-14 if I'm not mistaken.

@ghost
Copy link
Author

ghost commented Aug 31, 2023

I'd be fine mandating that for a build env. The OQS main CI build env is "Ubuntu Jammy" -- and that's at clang-14 if I'm not mistaken.

Exactly.

I've updated the CircleCI job to use clang-14 from "Ubuntu Jammy".
I had to disable PPIndentWidth because it seems broken in 14 but fixed in 15. So we won't be able to enforce the following:

Pre-processor directives use one space for indents:

@ghost ghost marked this pull request as ready for review August 31, 2023 14:05
@baentsch
Copy link
Member

So we won't be able to enforce the following:

Pre-processor directives use one space for indents:

Tough luck.

oqsprov/oqsprov.c Outdated Show resolved Hide resolved
oqsprov/oqsprov.c Outdated Show resolved Hide resolved
oqsprov/oqsprov_bio.c Outdated Show resolved Hide resolved
Copy link
Member

@baentsch baentsch left a comment

Choose a reason for hiding this comment

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

In general, very good: Looks like OpenSSL code. In most areas formatting (and readability) improved; in some it really became awful (see single comments): Is there something that could be done about those? OK, it's generated code, but still, it should be somewhat legible to humans, no?

@ghost
Copy link
Author

ghost commented Aug 31, 2023

Is there something that could be done about those? OK, it's generated code, but still, it should be somewhat legible to humans, no?

I agree. We can disable clang-format for some portions of the code.
Let me try something about this generated code.

@ghost
Copy link
Author

ghost commented Aug 31, 2023

@baentsch when I run the generate.py script, I got a diff on ALGORITHMS.md and on other files:

@@ -34,8 +34,6 @@ As standardization for these algorithms 
 | kyber768 | 0x023C | Yes | OQS_CODEPOINT_KYBER768 |
 | p384_kyber768 | 0x2F3C | Yes | OQS_CODEPOINT_P384_KYBER768 |
 | x448_kyber768 | 0x2F90 | Yes | OQS_CODEPOINT_X448_KYBER768 |
-| x25519_kyber768 | 0x6399 | Yes | OQS_CODEPOINT_X25519_KYBER768 |
-| p256_kyber768 | 0x639A | Yes | OQS_CODEPOINT_P256_KYBER768 |
 | kyber1024 | 0x023D | Yes | OQS_CODEPOINT_KYBER1024 |
 | p521_kyber1024 | 0x2F3D | Yes | OQS_CODEPOINT_P521_KYBER1024 |
 | bikel1 | 0x0241 | Yes | OQS_CODEPOINT_BIKEL1 |

I did:

$ OQS_ALGS_ENABLED=STD LIBOQS_SRC_DIR=../liboqs python3 oqs-template/generate.py

I tried without OQS_ALGS_ENABLED and I get the same. My liboqs is on main.
Do you have any idea why?

Edit: running the script on linux is fine, but not on macOS.

@ghost
Copy link
Author

ghost commented Aug 31, 2023

I added a // clang-format off / // clang-format on surrounding the oqsprov.c/kem_functions.fragment file. It should fix your two comments about the weird indentation of KEMHYBALGs.

I also removed the // #include "internal/cryptlib.h".

289bff8

@ghost ghost requested a review from baentsch August 31, 2023 15:05
@baentsch
Copy link
Member

Do you have any idea why?

Not really -- this is (should be) all driven off "generate.yml".

Edit: running the script on linux is fine, but not on macOS.

Are you sure "generate.yml" is identical on both machines? I just ran the script on macOS and nothing changed (i.e., x25519_kyber768 and p256_kyber768 "persisted").

@ghost
Copy link
Author

ghost commented Aug 31, 2023

Are you sure "generate.yml" is identical on both machines? I just ran the script on macOS and nothing changed (i.e., x25519_kyber768 and p256_kyber768 "persisted").

OK, it must have been because now it works. Sorry for the noise…

oqsprov/oqsprov.c Outdated Show resolved Hide resolved
… style.

This commit adds a `.clang-format` file at the root of the repository. This
`.clang-format` file tries to match the best the OpenSSL coding style.

This commit also reformats the existing code according to that `.clang-format`.

Finally, it adds a new CircleCI job to ensure that the code is well-formatted.
@ghost ghost requested a review from baentsch August 31, 2023 15:37
@baentsch baentsch merged commit 3750149 into open-quantum-safe:main Sep 1, 2023
@ghost ghost deleted the clang-format branch September 1, 2023 06:49
feventura pushed a commit to EntrustCorporation/oqs-provider that referenced this pull request Mar 16, 2024
…he OpenSSL coding style. (open-quantum-safe#241)

This commit adds a `.clang-format` file at the root of the repository. This
`.clang-format` file tries to match the best the OpenSSL coding style.

This commit also reformats the existing code according to that `.clang-format`.

Finally, it adds a new CircleCI job to ensure that the code is well-formatted.

Signed-off-by: Felipe Ventura <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant