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

Document the new coding style #69

Merged

Conversation

gilles-peskine-arm
Copy link
Contributor

@gilles-peskine-arm gilles-peskine-arm commented Dec 22, 2022

Update the coding standards document to the new coding style (including Mbed-TLS/mbedtls#6836).

To be merged at the same time as the new coding style.

Fixes Mbed-TLS/mbedtls#6603.

Provide the same amount of information with the same general structure, but
adapt the level of detail to what's unusual and what isn't.

Signed-off-by: Gilles Peskine <[email protected]>
Signed-off-by: Gilles Peskine <[email protected]>
We're calling the code formatting script code_style, so avoid using the
expressions "code style" or "coding style" to mean a different aspect of how
the code is written.

Signed-off-by: Gilles Peskine <[email protected]>
@gilles-peskine-arm gilles-peskine-arm added the needs-preceding-pr Requires another PR to be merged first label Dec 22, 2022
@mpg mpg self-requested a review December 23, 2022 08:47
Copy link
Contributor

@mpg mpg left a comment

Choose a reason for hiding this comment

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

Looking pretty good to me, just a couple minor points.

kb/development/mbedtls-coding-standards.md Outdated Show resolved Hide resolved
kb/development/mbedtls-coding-standards.md Show resolved Hide resolved
kb/development/mbedtls-coding-standards.md Outdated Show resolved Hide resolved
Signed-off-by: Gilles Peskine <[email protected]>
Signed-off-by: Gilles Peskine <[email protected]>
Now that the rule for opening braces isn't a simple "on its own line",
discuss the various cases: statements, types, initializers. In particular,
for statements, prominently mention the exception for function definitions.

Signed-off-by: Gilles Peskine <[email protected]>
mpg
mpg previously approved these changes Dec 23, 2022
Copy link
Contributor

@mpg mpg left a comment

Choose a reason for hiding this comment

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

LGTM! Thanks for writing the documentation in a way that's both clear, complete and concise!

* Inside parentheses: `if (f(sizeof(int), (int) y))`
* Inside square brackets: `a[i + 1]`
* Before a comma: `f(x, y)`
* After a unary operator: `if (!condition)`, `++x`

Choose a reason for hiding this comment

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

Suggested change
* After a unary operator: `if (!condition)`, `++x`
* After a prefix unary operator: `if (!condition)`, `++x`

* Inside square brackets: `a[i + 1]`
* Before a comma: `f(x, y)`
* After a unary operator: `if (!condition)`, `++x`
* Before a unary operator: `x++`

Choose a reason for hiding this comment

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

Suggested change
* Before a unary operator: `x++`
* Before a postfix unary operator: `x++`

mbedtls_write_thing( void *thing, unsigned char *buf, size_t buflen,
size_t *outlen ); // yes
mbedtls_write_thing(..., unsigned char *buf, size_t *len); // no
mbedtls_write_thing(..., unsigned char *buf, size_t buflen, size_t *outlen); // yes
```

For PSA functions, [input buffers](https://armmbed.github.io/mbed-crypto/html/overview/conventions.html#input-buffer-sizes) have a `size_t xxx_size` parameter after the buffer pointer, and [output buffers](https://armmbed.github.io/mbed-crypto/html/overview/conventions.html#output-buffer-sizes) have a `size_t xxx_size` parameter for the buffer size followed by a `size_t *xxx_length` parameter for the output length. This convention is also preferred in new `mbedtls_xxx` code, but older modules often use different conventions.

Choose a reason for hiding this comment

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

Can we link to the canonical PSA API pages instead? But maybe that should be a different PR...

The 1.1 specification is at https://arm-software.github.io/psa-api/crypto/1.1/

Copy link
Contributor Author

Choose a reason for hiding this comment

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

any update on having a latest link?

Copy link
Contributor

@mpg mpg left a comment

Choose a reason for hiding this comment

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

Still looks good to me with the changes since my previous review.

@gilles-peskine-arm gilles-peskine-arm merged commit 917c1da into Mbed-TLS:main Jan 10, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs-preceding-pr Requires another PR to be merged first needs-review needs-reviewer priority-high size-s Estimated task size: small (~2d)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Coding style change: update coding guidelines
3 participants