[Docs] Improve and unify Doxygen comments, part 1

[mkow]

Description of the changes

Please check if it looks good, if so then I'll update the rest of the docs (probably in parts).

---

This change is 

[dimakuv]

Reviewed 8 of 8 files at r1, all commit messages.
Reviewable status: all files reviewed, 3 unresolved discussions, not enough approvals from maintainers (2 more required), not enough approvals from different teams (1 more required, approved so far: ITL) (waiting on @mkow)

---

common/include/toml_utils.h, line 33 at r1 (raw file):

/*!
 * \brief  Find an integer key-value in TOML manifest.

Finds

---

common/include/toml_utils.h, line 46 at r1 (raw file):

/*!
 * \brief  Find a string key-value in TOML manifest.

Finds

---

common/include/toml_utils.h, line 58 at r1 (raw file):

/*!
 * \brief  Find a "size" string key-value in TOML manifest (parsed via `parse_size_str()`).

Finds

---

Documentation/devel/howto-doc.rst, line 159 at r1 (raw file):

       *
       * This function returns a number augmented by the Answer to the Ultimate
       * Question of Life, the Universe, and Everything.

I'll miss this reference to Adams, but I see why you chose a different example.

[mkow]

Reviewable status: 7 of 8 files reviewed, 3 unresolved discussions, not enough approvals from maintainers (2 more required), not enough approvals from different teams (1 more required, approved so far: ITL), "fixup! " found in commit messages' one-liners (waiting on @dimakuv)

---

common/include/toml_utils.h, line 33 at r1 (raw file):

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

Finds

Done.

---

common/include/toml_utils.h, line 46 at r1 (raw file):

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

Finds

Done.

---

common/include/toml_utils.h, line 58 at r1 (raw file):

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

Finds

Done.

---

Documentation/devel/howto-doc.rst, line 159 at r1 (raw file):

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

I'll miss this reference to Adams, but I see why you chose a different example.

Yeah, unfortunately it wasn't really helpful/informative here.

[dimakuv]

Reviewed 1 of 1 files at r2, all commit messages.
Reviewable status: all files reviewed, all discussions resolved, not enough approvals from maintainers (1 more required), "fixup! " found in commit messages' one-liners

[pwmarcz]

Reviewed 1 of 8 files at r1.
Reviewable status: all files reviewed, 4 unresolved discussions, not enough approvals from maintainers (1 more required), "fixup! " found in commit messages' one-liners (waiting on @mkow)

---

common/include/api.h, line 211 at r2 (raw file):

 * \param[out]  out_end    On success, set to the rest of string.
 *
 * \returns  0 on success, negative on failure.

I would prefer -1, not "negative". Most of our functions return negative error codes (POSIX or PAL...) on failure, some don't distinguish between errors and return only -1, I think it's better to be explicit and say we return -1 here.

---

common/include/spinlock.h, line 10 at r2 (raw file):

#define _SPINLOCK_H

#include <errno.h>

<errno.h> is from glibc (and defines the errno global/macro), I think in no-stdlib setting we should include <asm/errno.h>.

---

Documentation/devel/howto-doc.rst, line 162 at r2 (raw file):

       *
       * \returns  Sum of the arguments. Sometimes a longer description is needed,
       *           then is should be wrapped and aligned like this.

Typo: is -> it

---

Pal/src/host/Linux-SGX/enclave_ocalls.c, line 93 at r2 (raw file):

     *   (OCALL is not done, ret == -EAGAIN, let's wait on futex) */

    if (ret == -EAGAIN) {

I don't like this, it suggests there might be other errors that we will ignore. Either we keep open the possibility of other errors than -EAGAIN, and do something to handle them here, or (IMO better in this case) spinlock_lock_timeout should still return only 0 or -1.

(I don't like this timedout, but my preferred refactoring would be to keep the -1 return code, and check if (ret < 0) here).

[mkow]

Reviewable status: 5 of 8 files reviewed, 3 unresolved discussions, not enough approvals from maintainers (2 more required), not enough approvals from different teams (1 more required, approved so far: ITL), "fixup! " found in commit messages' one-liners (waiting on @dimakuv and @pwmarcz)

---

common/include/api.h, line 211 at r2 (raw file):

Previously, pwmarcz (Paweł Marczewski) wrote…

I would prefer -1, not "negative". Most of our functions return negative error codes (POSIX or PAL...) on failure, some don't distinguish between errors and return only -1, I think it's better to be explicit and say we return -1 here.

I was mostly afraid that if we say in some functions that -1 means an error, then it's quite easy to accidentally forward a retval from a subfunction and then do if (ret == -1) in another place, resulting in not noticing the failure. So, I'd prefer to have uniform "negative means failure" everywhere.

Ad. POSIX vs PAL vs random error codes: I plan to make them type-safe, I did some experiments and it seems to be possible in C with enough warnings enabled (see https://godbolt.org/z/zMvb3Mqar), so, soon there should be no risk of interpreting -1 as some enum.

---

common/include/spinlock.h, line 10 at r2 (raw file):

Previously, pwmarcz (Paweł Marczewski) wrote…

<errno.h> is from glibc (and defines the errno global/macro), I think in no-stdlib setting we should include <asm/errno.h>.

Done (removed because of the other discussion).

---

Documentation/devel/howto-doc.rst, line 162 at r2 (raw file):

Previously, pwmarcz (Paweł Marczewski) wrote…

Typo: is -> it

Heh, I read this 3 times and it still wasn't enough to spot this :)

---

Pal/src/host/Linux-SGX/enclave_ocalls.c, line 93 at r2 (raw file):

Previously, pwmarcz (Paweł Marczewski) wrote…

I don't like this, it suggests there might be other errors that we will ignore. Either we keep open the possibility of other errors than -EAGAIN, and do something to handle them here, or (IMO better in this case) spinlock_lock_timeout should still return only 0 or -1.

(I don't like this timedout, but my preferred refactoring would be to keep the -1 return code, and check if (ret < 0) here).

Ok, makes sense.

[pwmarcz]

Reviewed 3 of 8 files at r1, 1 of 1 files at r2, 3 of 3 files at r3, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion, not enough approvals from maintainers (2 more required), not enough approvals from different teams (1 more required, approved so far: ITL), "fixup! " found in commit messages' one-liners (waiting on @mkow)

---

common/include/api.h, line 211 at r2 (raw file):

Previously, mkow (Michał Kowalczyk) wrote…

I was mostly afraid that if we say in some functions that -1 means an error, then it's quite easy to accidentally forward a retval from a subfunction and then do if (ret == -1) in another place, resulting in not noticing the failure. So, I'd prefer to have uniform "negative means failure" everywhere.

Ad. POSIX vs PAL vs random error codes: I plan to make them type-safe, I did some experiments and it seems to be possible in C with enough warnings enabled (see https://godbolt.org/z/zMvb3Mqar), so, soon there should be no risk of interpreting -1 as some enum.

OK, I understand why we might want not to promise -1 exactly. But I'm still worried that somebody might forward this value (e.g. return ret;) thinking it's a POSIX error code.

So I would prefer something else than "negative": in some places we say "negative error code", so which sounds really similar. But I don't have a better suggestion, so I'm unblocking.

---

common/include/spinlock.h, line 85 at r3 (raw file):

 * \brief Try to acquire spinlock.
 *
 * \returns  0 if acquiring the lock succeeded, 1 if it was already taken.

Can you change this function to return -1 for consistency with the one below?

(This is unused btw, maybe we don't need it?)

[mkow]

Reviewable status: 7 of 8 files reviewed, 1 unresolved discussion, not enough approvals from maintainers (2 more required), not enough approvals from different teams (1 more required, approved so far: ITL), "fixup! " found in commit messages' one-liners (waiting on @pwmarcz)

---

common/include/spinlock.h, line 85 at r3 (raw file):

Previously, pwmarcz (Paweł Marczewski) wrote…

Can you change this function to return -1 for consistency with the one below?

(This is unused btw, maybe we don't need it?)

Good catch!
Not sure whether to keep it or remove, it's a library and having such API probably makes sense overall...

[pwmarcz]

Reviewed 1 of 1 files at r4, all commit messages.
Reviewable status: all files reviewed, all discussions resolved, not enough approvals from maintainers (1 more required), not enough approvals from different teams (1 more required, approved so far: ITL), "fixup! " found in commit messages' one-liners

[boryspoplawski]

Reviewed 3 of 8 files at r1, 2 of 3 files at r3, 1 of 1 files at r4, 2 of 2 files at r5, all commit messages.
Reviewable status: all files reviewed, all discussions resolved, not enough approvals from maintainers (2 more required), not enough approvals from different teams (1 more required, approved so far: ITL)

a discussion (no related file):
I personally find two spaces after \param redundant (unlike after arg name, where it looks nice) - it does not improve readability and just takes space.

---

a discussion (no related file):
I've put all comments as non-blocking, since it's late in the review process and already rebased.

---

---

common/include/api.h, line 211 at r2 (raw file):

Ad. POSIX vs PAL vs random error codes: I plan to make them type-safe, I did some experiments and it seems to be possible in C with enough warnings enabled (see https://godbolt.org/z/zMvb3Mqar), so, soon there should be no risk of interpreting -1 as some enum.

It's not so easy. If you do enum { A=1; B; }, then we would have to return -A and we get no type checking. If you do enum { A = -1;} then we get an ugly convention like return PAL_ERROR_AAAA; and you don't even know if it's positive or negative (check mbedtls sources for how it looks, they use that convention).

OK, I understand why we might want not to promise -1 exactly. But I'm still worried that some1body might forward this value (e.g. return ret;) thinking it's a POSIX error code.

Fortunately 1 is EPERM so nothing bad should happen (it's not like EINTR or something).

---

common/include/api.h, line 204 at r5 (raw file):

/*!
 * \brief  Converts a string to number.

Why did you change all \brief to 3rd person? Old version matched our conventions (e.g. commit message, which describes commit content, just like this comment describes a function).
ditto for the whole PR

---

common/include/spinlock.h, line 85 at r3 (raw file):

Previously, mkow (Michał Kowalczyk) wrote…

Good catch!
Not sure whether to keep it or remove, it's a library and having such API probably makes sense overall...

I would vote for removing it.

---

common/include/spinlock.h, line 126 at r5 (raw file):

 * \param  iterations  Number of iterations (tries) after which this function times out.
 *
 * \returns  0 if acquiring the lock succeeded, negative if timed out.

Do we treat timeout as an error (since you've changed to a negative value)? I thought of it as just another possible outcome of this function, which is not an error, so 1 (positive value) made sense.

Update: after looking at usages - this should just return bool.

---

common/src/printf.c, line 59 at r5 (raw file):

 * \brief  Core printf implementation.
 *
 * \param       write_callback  Function called on each generated chunk of data.

I think the old version (no capital letter and no dot at the end) looks better when rendered in docs.

---

common/src/printf.c, line 64 at r5 (raw file):

 * \param       ap              List of optional variadic arguments.
 * \param[out]  out_size        Total size of written data (sum of sizes passed to all \p
 *                              write_callback invocations).

I would move \p to the new line. It probably renders ok, but is weird to read.

---

Documentation/devel/howto-doc.rst, line 161 at r5 (raw file):

       * \param  second  Second addend.
       *
       * \returns  Sum of the arguments. Sometimes a longer description is needed,

Is it grammatically correct? I would change Sometimes -> If/When

---

Documentation/devel/howto-doc.rst, line 161 at r5 (raw file):

       * \param  second  Second addend.
       *
       * \returns  Sum of the arguments. Sometimes a longer description is needed,

This is not correctly wrapped (line length).

[pwmarcz]

Reviewed 2 of 2 files at r5, all commit messages.
Reviewable status: all files reviewed, all discussions resolved, not enough approvals from maintainers (2 more required), not enough approvals from different teams (1 more required, approved so far: ITL)

a discussion (no related file):

Previously, boryspoplawski (Borys Popławski) wrote…

I personally find two spaces after \param redundant (unlike after arg name, where it looks nice) - it does not improve readability and just takes space.

I think I asked for it at some point. It looks more readable to me, but I don't care too much.

But keep in mind it should be consistent with spacing after \returns and \brief. This PR mostly does that, but I see at least of \brief with one space (in spinlock.h).

---

---

common/include/api.h, line 204 at r5 (raw file):

Previously, boryspoplawski (Borys Popławski) wrote…

Why did you change all \brief to 3rd person? Old version matched our conventions (e.g. commit message, which describes commit content, just like this comment describes a function).
ditto for the whole PR

Actually, yes, +1 to that. The current style (imperative \brief, longer description in 3rd person) looks natural to me.

[dimakuv]

Reviewed 2 of 3 files at r3, 1 of 1 files at r4, 2 of 2 files at r5, all commit messages.
Reviewable status: all files reviewed, all discussions resolved, not enough approvals from maintainers (1 more required)

---

common/include/api.h, line 204 at r5 (raw file):

Previously, pwmarcz (Paweł Marczewski) wrote…

Actually, yes, +1 to that. The current style (imperative \brief, longer description in 3rd person) looks natural to me.

I don't care, so whatever you decide on.

[boryspoplawski]

Reviewable status: all files reviewed, all discussions resolved, not enough approvals from maintainers (1 more required)

a discussion (no related file):

Previously, pwmarcz (Paweł Marczewski) wrote…

I think I asked for it at some point. It looks more readable to me, but I don't care too much.

But keep in mind it should be consistent with spacing after \returns and \brief. This PR mostly does that, but I see at least of \brief with one space (in spinlock.h).

I would remove it from \ret and \brief too - I believe the original intent was to make \param arg something less confusing, so that you don't mistake arg as part of the sentence

---

[mkow]

Reviewable status: 6 of 8 files reviewed, 1 unresolved discussion, not enough approvals from maintainers (2 more required), not enough approvals from different teams (1 more required, approved so far: ITL), "fixup! " found in commit messages' one-liners (waiting on @boryspoplawski, @dimakuv, @mkow, and @pwmarcz)

a discussion (no related file):

Previously, boryspoplawski (Borys Popławski) wrote…

I would remove it from \ret and \brief too - I believe the original intent was to make \param arg something less confusing, so that you don't mistake arg as part of the sentence

I'm rather neutral on this, but it should be either a double space after all \tags or always single (as Borys proposed).

@dimakuv What's your preference?

---

a discussion (no related file):

Previously, boryspoplawski (Borys Popławski) wrote…

I've put all comments as non-blocking, since it's late in the review process and already rebased.

No hurry, let's resolve them first.

---

---

common/include/api.h, line 211 at r2 (raw file):

Previously, boryspoplawski (Borys Popławski) wrote…

Ad. POSIX vs PAL vs random error codes: I plan to make them type-safe, I did some experiments and it seems to be possible in C with enough warnings enabled (see https://godbolt.org/z/zMvb3Mqar), so, soon there should be no risk of interpreting -1 as some enum.

It's not so easy. If you do enum { A=1; B; }, then we would have to return -A and we get no type checking. If you do enum { A = -1;} then we get an ugly convention like return PAL_ERROR_AAAA; and you don't even know if it's positive or negative (check mbedtls sources for how it looks, they use that convention).

OK, I understand why we might want not to promise -1 exactly. But I'm still worried that some1body might forward this value (e.g. return ret;) thinking it's a POSIX error code.

Fortunately 1 is EPERM so nothing bad should happen (it's not like EINTR or something).

Hmm, good point, not sure what to do with the negative values...

---

common/include/api.h, line 204 at r5 (raw file):

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

I don't care, so whatever you decide on.

I think for functions it feels more natural to use this form, because the comment describes what a function does "in general", without binding to a specific point in time. With commits it's a bit different, commit history is more like a list of changes/commands applied in order to the source.

---

common/include/spinlock.h, line 85 at r3 (raw file):

Previously, boryspoplawski (Borys Popławski) wrote…

I would vote for removing it.

Ok, done.

---

common/include/spinlock.h, line 126 at r5 (raw file):

Previously, boryspoplawski (Borys Popławski) wrote…

Do we treat timeout as an error (since you've changed to a negative value)? I thought of it as just another possible outcome of this function, which is not an error, so 1 (positive value) made sense.

Update: after looking at usages - this should just return bool.

Yeah, I consider it to be an equivalent of -EAGAIN - function failed to do what was asked, and the reason for failure is a timeout.
Bool also sounds ok for me, but let's ask @pwmarcz what he thinks about it.

---

common/src/printf.c, line 59 at r5 (raw file):

Previously, boryspoplawski (Borys Popławski) wrote…

I think the old version (no capital letter and no dot at the end) looks better when rendered in docs.

I agree, but tbh I mostly read this directly in the code, where the capital letter help a lot visually.
@pwmarcz, @dimakuv: Any opinions on this?

---

common/src/printf.c, line 64 at r5 (raw file):

Previously, boryspoplawski (Borys Popławski) wrote…

I would move \p to the new line. It probably renders ok, but is weird to read.

Good point, done.

---

Documentation/devel/howto-doc.rst, line 161 at r5 (raw file):

Previously, boryspoplawski (Borys Popławski) wrote…

Is it grammatically correct? I would change Sometimes -> If/When

Hmm, I think it's correct, but maybe changing then-> in such a case would help?

[pwmarcz]

Reviewed all commit messages.
Reviewable status: 6 of 8 files reviewed, 1 unresolved discussion, not enough approvals from maintainers (2 more required), not enough approvals from different teams (1 more required, approved so far: ITL), "fixup! " found in commit messages' one-liners (waiting on @boryspoplawski, @dimakuv, @mkow, and @pwmarcz)

---

common/include/api.h, line 204 at r5 (raw file):

Previously, mkow (Michał Kowalczyk) wrote…

I think for functions it feels more natural to use this form, because the comment describes what a function does "in general", without binding to a specific point in time. With commits it's a bit different, commit history is more like a list of changes/commands applied in order to the source.

Looking for precedents:

• I see Doxygen examples with 3rd person
• Linux doesn't seem to use a consistent style (their guide says just to write a "brief description")
• Xen project seems to prefer the current style (brief in 2nd person, description in 3rd person)
• Linux and BSD man pages use the current style (brief: close - close a file descriptor, then later close() closes a file descriptor, so that...)
• Rust uses 3rd person only (e.g. pub fn clear(...) - Truncates this String, removing all contents.), but doesn't seem to have any "brief" section at all, just a paragraph of description)
• Ruby also uses 3rd person only (e.g. clear -> self - Remove the contents of self), but also doesn't have a brief section
• Python ([https://www.python.org/dev/peps/pep-0257/](PEP 257)) doesn't talk about the grammatical form, but has examples in the imperative (e.g. Return the pathname..., Do X and return a list.)
• Javadoc style guide explicitly says to use 3rd person, not imperative, but AFAIK also doesn't describe a separate "brief" section

To summarize: I have some examples where a "brief" section uses 2nd person imperative, and description uses 3rd person (Xen, Linux and BSD man pages, Python). Using 3rd person for the "brief" section seems pretty rare (although there are projects that do not have this line at all).

Most relevant/prominent seems to be the example of man pages.

[boryspoplawski]

Reviewed 2 of 2 files at r6, all commit messages.
Reviewable status: all files reviewed, 4 unresolved discussions, not enough approvals from maintainers (2 more required), not enough approvals from different teams (1 more required, approved so far: ITL), "fixup! " found in commit messages' one-liners (waiting on @dimakuv, @mkow, and @pwmarcz)

---

common/include/api.h, line 204 at r5 (raw file):

commit history is more like a list of changes/commands applied in order to the source.

Agreed, and commit message is describing these changes, or putting in your words: describes what a commit does "in general".

---

common/include/spinlock.h, line 126 at r5 (raw file):

Previously, mkow (Michał Kowalczyk) wrote…

Yeah, I consider it to be an equivalent of -EAGAIN - function failed to do what was asked, and the reason for failure is a timeout.
Bool also sounds ok for me, but let's ask @pwmarcz what he thinks about it.

In this case it's not really that function failed, we literally asked it to spin for a given timeout...

---

common/src/printf.c, line 59 at r5 (raw file):

Previously, mkow (Michał Kowalczyk) wrote…

I agree, but tbh I mostly read this directly in the code, where the capital letter help a lot visually.
@pwmarcz, @dimakuv: Any opinions on this?

With double space I don't mind this even in the source

[dimakuv]

Reviewed 2 of 2 files at r6, all commit messages.
Reviewable status: all files reviewed, 4 unresolved discussions, not enough approvals from maintainers (1 more required), "fixup! " found in commit messages' one-liners (waiting on @boryspoplawski, @mkow, and @pwmarcz)

a discussion (no related file):

Previously, mkow (Michał Kowalczyk) wrote…

I'm rather neutral on this, but it should be either a double space after all \tags or always single (as Borys proposed).

@dimakuv What's your preference?

I liked a single space for \ret and \brief. My opinion is the same as Borys's.

---

---

common/src/printf.c, line 59 at r5 (raw file):

Previously, boryspoplawski (Borys Popławski) wrote…

With double space I don't mind this even in the source

I like the capital letter.

[pwmarcz]

Reviewable status: all files reviewed, 4 unresolved discussions, not enough approvals from maintainers (1 more required), "fixup! " found in commit messages' one-liners (waiting on @boryspoplawski, @mkow, and @pwmarcz)

---

common/include/spinlock.h, line 126 at r5 (raw file):

Previously, boryspoplawski (Borys Popławski) wrote…

In this case it's not really that function failed, we literally asked it to spin for a given timeout...

I like the idea of using bool (true for success, right?). In other instances where we return -1 (mostly parsing) the failure is more obvious and unrecoverable.

As I said before, using -EAGAIN here kind of suggests that this function might fail in other ways which we should handle.

[pwmarcz]

Reviewable status: all files reviewed, 4 unresolved discussions, not enough approvals from maintainers (1 more required), "fixup! " found in commit messages' one-liners (waiting on @boryspoplawski and @mkow)

---

common/src/printf.c, line 59 at r5 (raw file):

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

I like the capital letter.

With the capital letter, it's easier to write multi-sentence parameter descriptions (but do we ever do that?) But I don't mind either way.

[boryspoplawski]

Reviewable status: all files reviewed, 4 unresolved discussions, not enough approvals from maintainers (1 more required), "fixup! " found in commit messages' one-liners (waiting on @boryspoplawski, @dimakuv, @mkow, and @pwmarcz)

---

common/include/spinlock.h, line 126 at r5 (raw file):

true for success, right?

Imo yes

As I said before, using -EAGAIN here kind of suggests that this function might fail in other ways which we should handle.

Yeah, and I wouldn't even call it a fail, it's a legitimate and expected timeout.

---

common/src/printf.c, line 59 at r5 (raw file):

Previously, pwmarcz (Paweł Marczewski) wrote…

With the capital letter, it's easier to write multi-sentence parameter descriptions (but do we ever do that?) But I don't mind either way.

I think we should not write multi-sentence descriptions here, they are supposed to be brief

[mkow]

Reviewable status: 0 of 8 files reviewed, 3 unresolved discussions, not enough approvals from maintainers (2 more required), not enough approvals from different teams (1 more required, approved so far: ITL), "fixup! " found in commit messages' one-liners (waiting on @boryspoplawski, @dimakuv, @mkow, and @pwmarcz)

a discussion (no related file):

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

I liked a single space for \ret and \brief. My opinion is the same as Borys's.

Ok, then I'm changing it to a single space. Please check the new version and see if it looks better.

---

---

common/include/api.h, line 204 at r5 (raw file):

commit history is more like a list of changes/commands applied in order to the source.

Agreed, and commit message is describing these changes, or putting in your words: describes what a commit does "in general".

Commit message body: yes (but for this I think we don't have a uniform convention). For one-liners it's IMO different, you usually view them as a log of changes done to the codebase, so we use imperative mood.

Anyways, this seem to be mostly a matter of taste, so let's just vote who likes which style more and use the most preferred one. I prefer 3rd person and Borys prefers imperative mood. @dimakuv, @pwmarcz: Please vote :)

---

common/include/spinlock.h, line 126 at r5 (raw file):

true for success, right?

Yup, definitely this way. Done.

Yeah, and I wouldn't even call it a fail, it's a legitimate and expected timeout.

I consider this as a failure. IMO if it could also fail for other reasons (e.g. OOM), then we'd just return -EAGAIN and -ENOMEM.

---

common/src/printf.c, line 59 at r5 (raw file):

Previously, boryspoplawski (Borys Popławski) wrote…

I think we should not write multi-sentence descriptions here, they are supposed to be brief

Sometimes we do when function semantic is non-trivial, see e.g. parse_size_str() in this very PR.

---

Documentation/devel/howto-doc.rst, line 161 at r5 (raw file):

Previously, boryspoplawski (Borys Popławski) wrote…

This is not correctly wrapped (line length).

Will fix depending on the resolution of the other discussion on this line.

[boryspoplawski]

Reviewed 8 of 8 files at r7, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion, not enough approvals from maintainers (2 more required), not enough approvals from different teams (1 more required, approved so far: ITL), "fixup! " found in commit messages' one-liners (waiting on @mkow)

---

common/include/api.h, line 204 at r5 (raw file):

Previously, mkow (Michał Kowalczyk) wrote…

commit history is more like a list of changes/commands applied in order to the source.

Agreed, and commit message is describing these changes, or putting in your words: describes what a commit does "in general".

Commit message body: yes (but for this I think we don't have a uniform convention). For one-liners it's IMO different, you usually view them as a log of changes done to the codebase, so we use imperative mood.

Anyways, this seem to be mostly a matter of taste, so let's just vote who likes which style more and use the most preferred one. I prefer 3rd person and Borys prefers imperative mood. @dimakuv, @pwmarcz: Please vote :)

From prev comments seems like @pwmarcz prefers imperative and @dimakuv doesn't care.

---

common/src/printf.c, line 59 at r5 (raw file):

Previously, mkow (Michał Kowalczyk) wrote…

Sometimes we do when function semantic is non-trivial, see e.g. parse_size_str() in this very PR.

One could argue that the 2nd sentence should be moved to the extended description below.

---

Documentation/devel/howto-doc.rst, line 161 at r5 (raw file):

Previously, mkow (Michał Kowalczyk) wrote…

Hmm, I think it's correct, but maybe changing then-> in such a case would help?

Idk, it just reads weird to me

[pwmarcz]

Reviewed 8 of 8 files at r7, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion, not enough approvals from maintainers (1 more required), not enough approvals from different teams (1 more required, approved so far: ITL), "fixup! " found in commit messages' one-liners (waiting on @mkow)

---

common/include/api.h, line 204 at r5 (raw file):

Previously, boryspoplawski (Borys Popławski) wrote…

From prev comments seems like @pwmarcz prefers imperative and @dimakuv doesn't care.

Yes, I prefer imperative for brief and 3rd person for description. Both as a matter of taste, and because it seems to be more common (when a project has a "brief" line, they generally use imperative for it).

[dimakuv]

Reviewed 8 of 8 files at r7, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion, "fixup! " found in commit messages' one-liners (waiting on @mkow)

---

common/include/api.h, line 204 at r5 (raw file):

Previously, pwmarcz (Paweł Marczewski) wrote…

Yes, I prefer imperative for brief and 3rd person for description. Both as a matter of taste, and because it seems to be more common (when a project has a "brief" line, they generally use imperative for it).

I don't care.

[mkow]

Reviewable status: 2 of 8 files reviewed, 1 unresolved discussion, not enough approvals from maintainers (1 more required), not enough approvals from different teams (1 more required, approved so far: ITL), "fixup! " found in commit messages' one-liners (waiting on @boryspoplawski, @dimakuv, @mkow, and @pwmarcz)

---

common/include/api.h, line 204 at r5 (raw file):

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

I don't care.

Ok, changed to imperative then.

---

common/src/printf.c, line 59 at r5 (raw file):

Previously, boryspoplawski (Borys Popławski) wrote…

One could argue that the 2nd sentence should be moved to the extended description below.

I prefer to keep it at the arg if it directly explain this very argument.

[boryspoplawski]

Reviewed 6 of 6 files at r8, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion, not enough approvals from maintainers (1 more required), not enough approvals from different teams (1 more required, approved so far: ITL), "fixup! " found in commit messages' one-liners (waiting on @mkow)

---

Documentation/devel/howto-doc.rst, line 161 at r5 (raw file):

Previously, mkow (Michał Kowalczyk) wrote…

Will fix depending on the resolution of the other discussion on this line.

Still incorrect?

[mkow]

Reviewable status: all files reviewed, 1 unresolved discussion, not enough approvals from maintainers (1 more required), not enough approvals from different teams (1 more required, approved so far: ITL), "fixup! " found in commit messages' one-liners (waiting on @mkow)

---

Documentation/devel/howto-doc.rst, line 161 at r5 (raw file):

Previously, boryspoplawski (Borys Popławski) wrote…

Still incorrect?

Then what's "correct"? You mean I should wrap like it was a source file and * was at position 0 in it?

[boryspoplawski]

Reviewable status: all files reviewed, 1 unresolved discussion, not enough approvals from maintainers (1 more required), not enough approvals from different teams (1 more required, approved so far: ITL), "fixup! " found in commit messages' one-liners (waiting on @mkow)

---

Documentation/devel/howto-doc.rst, line 161 at r5 (raw file):

Previously, mkow (Michał Kowalczyk) wrote…

Then what's "correct"? You mean I should wrap like it was a source file and * was at position 0 in it?

I meant (line length)- to wrap it to 100 chars - it's in .rst but it's an example C source.
Idk about position of *, but probably yes, assume it's 0, so like make line limit 108

[mkow]

Reviewable status: 7 of 8 files reviewed, 1 unresolved discussion, not enough approvals from maintainers (1 more required), not enough approvals from different teams (1 more required, approved so far: ITL), "fixup! " found in commit messages' one-liners (waiting on @boryspoplawski)

---

Documentation/devel/howto-doc.rst, line 161 at r5 (raw file):

Previously, boryspoplawski (Borys Popławski) wrote…

I meant (line length)- to wrap it to 100 chars - it's in .rst but it's an example C source.
Idk about position of *, but probably yes, assume it's 0, so like make line limit 108

Done.

[dimakuv]

Reviewed 5 of 6 files at r8, 1 of 1 files at r9, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion, "fixup! " found in commit messages' one-liners (waiting on @boryspoplawski)

---

Documentation/devel/howto-doc.rst, line 161 at r5 (raw file):

Previously, mkow (Michał Kowalczyk) wrote…

Done.

The line limit here is not 108, and I am very much opposed to this suggestion (if I understood Borys correctly). How am I supposed to set up my Vim to automatically figure out that some lines are 100 chars and some 108?

The current version has a limit of 100 chars, and I prefer to keep like this.

[pwmarcz]

Reviewed 5 of 6 files at r8, 1 of 1 files at r9, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion, "fixup! " found in commit messages' one-liners (waiting on @boryspoplawski)

---

Documentation/devel/howto-doc.rst, line 161 at r5 (raw file):

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

The line limit here is not 108, and I am very much opposed to this suggestion (if I understood Borys correctly). How am I supposed to set up my Vim to automatically figure out that some lines are 100 chars and some 108?

The current version has a limit of 100 chars, and I prefer to keep like this.

This is C source in our coding style (column limit 100) embedded inside reST (column limit 80), moved 6 characters to the right (notice the position of /* and int). I think two choices are appropriate:

1. Keep this wrapped at 80 characters, so that it's easy to edit in this file.
2. Keep this wrapped at 106 characters (not 108), so that the code block, which we display to the user, is wrapped to 100 characters.

I think the second choice is more "correct", since this is supposed to show our coding style. But it's also pretty inconvenient to edit, and maybe we need a (reST) comment explaining the column limit:

#. Prefer Qt-style ``/*!`` and ``\param``:

   ..
      Please keep the following code block wrapped at 100 characters (plus left margin):
      ....................................................................................................|

   .. code-block:: c

      /*!
       * \brief Sum two integers.

[dimakuv]

Reviewable status: all files reviewed, 1 unresolved discussion, "fixup! " found in commit messages' one-liners (waiting on @boryspoplawski)

---

Documentation/devel/howto-doc.rst, line 161 at r5 (raw file):

Previously, pwmarcz (Paweł Marczewski) wrote…

This is C source in our coding style (column limit 100) embedded inside reST (column limit 80), moved 6 characters to the right (notice the position of /* and int). I think two choices are appropriate:

1. Keep this wrapped at 80 characters, so that it's easy to edit in this file.
2. Keep this wrapped at 106 characters (not 108), so that the code block, which we display to the user, is wrapped to 100 characters.

I think the second choice is more "correct", since this is supposed to show our coding style. But it's also pretty inconvenient to edit, and maybe we need a (reST) comment explaining the column limit:

#. Prefer Qt-style ``/*!`` and ``\param``:

   ..
      Please keep the following code block wrapped at 100 characters (plus left margin):
      ....................................................................................................|

   .. code-block:: c

      /*!
       * \brief Sum two integers.

Why are we doing this? What will it solve? How do we verify this limit during code reviews?

I have a strong feeling we're doing some bike shedding here. Why not just go with the easiest option (a single char limit for each file type).

[pwmarcz]

Reviewable status: all files reviewed, 1 unresolved discussion, "fixup! " found in commit messages' one-liners (waiting on @boryspoplawski)

---

Documentation/devel/howto-doc.rst, line 161 at r5 (raw file):

Why are we doing this? What will it solve?

Make the example consistent with other recommendations. We ask contributors to wrap at 100 columns, they should see code wrapped at 100 columns here. (They will probably read the HTML, so they don't care about how the reST source looks like).

How do we verify this limit during code reviews?

To be clear, I'm not proposing to make a 106 column rule in such cases. I think we can make an exception for this code snippet (because it's supposed to show wrapping) and wrap everything else in our documentation at 80 columns. Hopefully, there won't be many snippets that even need wrapping.

But wrapping this one at 80 columns is fine for me, too.

[boryspoplawski]

Reviewed 1 of 1 files at r9, all commit messages.
Reviewable status: all files reviewed, all discussions resolved, "fixup! " found in commit messages' one-liners

---

Documentation/devel/howto-doc.rst, line 161 at r5 (raw file):

Previously, pwmarcz (Paweł Marczewski) wrote…

Why are we doing this? What will it solve?

Make the example consistent with other recommendations. We ask contributors to wrap at 100 columns, they should see code wrapped at 100 columns here. (They will probably read the HTML, so they don't care about how the reST source looks like).

How do we verify this limit during code reviews?

To be clear, I'm not proposing to make a 106 column rule in such cases. I think we can make an exception for this code snippet (because it's supposed to show wrapping) and wrap everything else in our documentation at 80 columns. Hopefully, there won't be many snippets that even need wrapping.

But wrapping this one at 80 columns is fine for me, too.

Yeah, typo, I meant 106 not 108.

@dimakuv as @pwmarcz explained the idea is that this is an C code example that users will read in HTML. If we left the old wrapping (80), then such example wouldn't go through a review when copy-pasted...

[dimakuv]

Reviewable status: all files reviewed, all discussions resolved, "fixup! " found in commit messages' one-liners

---

Documentation/devel/howto-doc.rst, line 161 at r5 (raw file):

Previously, boryspoplawski (Borys Popławski) wrote…

Yeah, typo, I meant 106 not 108.

@dimakuv as @pwmarcz explained the idea is that this is an C code example that users will read in HTML. If we left the old wrapping (80), then such example wouldn't go through a review when copy-pasted...

Ah, this discussion is purely for this particular "C code embedded in RST file" case. Thanks and sorry for confusion.

[pwmarcz]

Reviewable status: all files reviewed, all discussions resolved, "fixup! " found in commit messages' one-liners

---

Documentation/devel/howto-doc.rst, line 161 at r5 (raw file):

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

Ah, this discussion is purely for this particular "C code embedded in RST file" case. Thanks and sorry for confusion.

Not blocking, but @mkow please consider leaving a comment for the next person that tries to reformat this code :) (Either the one I suggested above, or something shorter)

[boryspoplawski]

Reviewable status: all files reviewed, 1 unresolved discussion, "fixup! " found in commit messages' one-liners (waiting on @pwmarcz)

---

Documentation/devel/howto-doc.rst, line 161 at r5 (raw file):

Previously, pwmarcz (Paweł Marczewski) wrote…

Not blocking, but @mkow please consider leaving a comment for the next person that tries to reformat this code :) (Either the one I suggested above, or something shorter)

+1 and blocking :)

[mkow]

Reviewable status: 7 of 8 files reviewed, 1 unresolved discussion, not enough approvals from maintainers (1 more required), not enough approvals from different teams (1 more required, approved so far: ITL), "fixup! " found in commit messages' one-liners (waiting on @boryspoplawski, @dimakuv, and @pwmarcz)

---

Documentation/devel/howto-doc.rst, line 161 at r5 (raw file):

Previously, boryspoplawski (Borys Popławski) wrote…

+1 and blocking :)

Added.

[boryspoplawski]

Reviewed 1 of 1 files at r10, all commit messages.
Reviewable status: all files reviewed, all discussions resolved, not enough approvals from different teams (1 more required, approved so far: ITL), "fixup! " found in commit messages' one-liners

[pwmarcz]

Reviewed 1 of 1 files at r10, all commit messages.
Reviewable status: all files reviewed, all discussions resolved, not enough approvals from different teams (1 more required, approved so far: ITL), "fixup! " found in commit messages' one-liners

[boryspoplawski]

Reviewed all commit messages.
Reviewable status: all files reviewed, all discussions resolved, not enough approvals from different teams (1 more required, approved so far: ITL)

[pwmarcz]

Reviewed all commit messages.
Reviewable status: all files reviewed, all discussions resolved, not enough approvals from different teams (1 more required, approved so far: ITL)

[dimakuv]

Reviewed 1 of 1 files at r10, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved