[libvirt PATCH 0/4] Code style documentation
Some additions and clarifications to libvirt's code style documentation, based on points of feedback that are given regularly on the mailing list. Tim Wiederhake (4): docs: coding-style: Clarify on virXXXPtr types docs: coding-style: Rewrite section on shortening comparisons docs: coding-style: Remove "no_memory" as acceptable goto target docs: coding-style: One variable declaration per line docs/coding-style.rst | 79 ++++++++++++++++++++++++++++++------------- 1 file changed, 56 insertions(+), 23 deletions(-) -- 2.31.1
Signed-off-by: Tim Wiederhake <twiederh@redhat.com> --- docs/coding-style.rst | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/docs/coding-style.rst b/docs/coding-style.rst index 470c61860f..dca9de1915 100644 --- a/docs/coding-style.rst +++ b/docs/coding-style.rst @@ -54,6 +54,7 @@ Struct type names and each following word should have its first letter in uppercase. The struct name should be the same as the typedef name with a leading underscore. + :: typedef struct _virHashTable virHashTable; @@ -61,6 +62,10 @@ Struct type names ... }; + Historically, libvirt declared pointer types 'virXXXPtr' for + both public and internal types. Do not introduce new such + typedefs for internal types. + Function names All functions should have a 'vir' prefix in their name, followed by one or more words with first letter of each word -- 2.31.1
On a Friday in 2022, Tim Wiederhake wrote:
Signed-off-by: Tim Wiederhake <twiederh@redhat.com> --- docs/coding-style.rst | 5 +++++ 1 file changed, 5 insertions(+)
diff --git a/docs/coding-style.rst b/docs/coding-style.rst index 470c61860f..dca9de1915 100644 --- a/docs/coding-style.rst +++ b/docs/coding-style.rst @@ -54,6 +54,7 @@ Struct type names and each following word should have its first letter in uppercase. The struct name should be the same as the typedef name with a leading underscore. + ::
typedef struct _virHashTable virHashTable; @@ -61,6 +62,10 @@ Struct type names ... };
+ Historically, libvirt declared pointer types 'virXXXPtr' for + both public and internal types. Do not introduce new such + typedefs for internal types. +
This weakly suggests that they should be introduced for new public types. I have no preference, but I think we should mention it here if we're mentioning internal types. Jano
Function names All functions should have a 'vir' prefix in their name, followed by one or more words with first letter of each word -- 2.31.1
On Fri, Jan 14, 2022 at 04:43:58PM +0100, Ján Tomko wrote:
On a Friday in 2022, Tim Wiederhake wrote:
Signed-off-by: Tim Wiederhake <twiederh@redhat.com> --- docs/coding-style.rst | 5 +++++ 1 file changed, 5 insertions(+)
diff --git a/docs/coding-style.rst b/docs/coding-style.rst index 470c61860f..dca9de1915 100644 --- a/docs/coding-style.rst +++ b/docs/coding-style.rst @@ -54,6 +54,7 @@ Struct type names and each following word should have its first letter in uppercase. The struct name should be the same as the typedef name with a leading underscore. + ::
typedef struct _virHashTable virHashTable; @@ -61,6 +62,10 @@ Struct type names ... };
+ Historically, libvirt declared pointer types 'virXXXPtr' for + both public and internal types. Do not introduce new such + typedefs for internal types.
This weakly suggests that they should be introduced for new public types. I have no preference, but I think we should mention it here if we're mentioning internal types.
-- Andrea Bolognani / Red Hat / Virtualization
On Fri, Jan 14, 2022 at 04:43:58PM +0100, Ján Tomko wrote:
On a Friday in 2022, Tim Wiederhake wrote:
+ Historically, libvirt declared pointer types 'virXXXPtr' for + both public and internal types. Do not introduce new such + typedefs for internal types.
This weakly suggests that they should be introduced for new public types. I have no preference, but I think we should mention it here if we're mentioning internal types.
Agreed. More specifically, I think we should encourage defining the Ptr alias for public types because, much as I dislike their existence, having a mix of both conventions in the public API would be IMO worse than sticking with the status quo. -- Andrea Bolognani / Red Hat / Virtualization
On Fri, Jan 14, 2022 at 09:24:23AM -0800, Andrea Bolognani wrote:
On Fri, Jan 14, 2022 at 04:43:58PM +0100, Ján Tomko wrote:
On a Friday in 2022, Tim Wiederhake wrote:
+ Historically, libvirt declared pointer types 'virXXXPtr' for + both public and internal types. Do not introduce new such + typedefs for internal types.
This weakly suggests that they should be introduced for new public types. I have no preference, but I think we should mention it here if we're mentioning internal types.
Agreed. More specifically, I think we should encourage defining the Ptr alias for public types because, much as I dislike their existence, having a mix of both conventions in the public API would be IMO worse than sticking with the status quo.
Yes, the public API must carry on using its existing practice. Only the internal usage is eliminated. Regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
The code style showed `bool hasFoos; if (hasFoos == true)` as a good example in one place, only to warn against comparisons with `true` a couple of paragraphs further down. Merge this advice on comparing with `true` into the "Conditional expressions" section and split the example up for readability. Signed-off-by: Tim Wiederhake <twiederh@redhat.com> --- docs/coding-style.rst | 60 +++++++++++++++++++++++++++---------------- 1 file changed, 38 insertions(+), 22 deletions(-) diff --git a/docs/coding-style.rst b/docs/coding-style.rst index dca9de1915..3dedb032f4 100644 --- a/docs/coding-style.rst +++ b/docs/coding-style.rst @@ -427,25 +427,47 @@ Conditional expressions ----------------------- For readability reasons new code should avoid shortening -comparisons to 0 for numeric types. Boolean and pointer -comparisons may be shortened. All long forms are okay: +comparisons to 0 for numeric types: :: - virFoo *foos = NULL; size nfoos = 0; - bool hasFoos = false; GOOD: - if (!foos) - if (!hasFoos) + if (nfoos != 0) if (nfoos == 0) - if (foos == NULL) - if (hasFoos == true) BAD: - if (!nfoos) if (nfoos) + if (!nfoos) + +Prefer the shortened version for boolean values. Boolean values +should never be compared against the literal ``true``, as a +logical non-false value need not be ``1``. + +:: + + bool hasFoos = false; + + GOOD: + if (hasFoos) + if (!hasFoos) + + BAD: + if (hasFoos == true) + if (hasFoos != false) + if (hasFoos == false) + if (hasFoos != true) + +Pointer comparisons may be shortened. All long forms are okay. + +:: + + virFoo *foo = NULL; + + GOOD: + if (foo) # or: if (foo != NULL) + if (!foo) # or: if (foo == NULL) New code should avoid the ternary operator as much as possible. Specifically it must never span more than one line or nest: @@ -507,19 +529,13 @@ Scalars - In the unusual event that you require a specific width, use a standard type like ``int32_t``, ``uint32_t``, ``uint64_t``, etc. -- While using ``bool`` is good for readability, it comes with - minor caveats: - - - Don't use ``bool`` in places where the type size must be - constant across all systems, like public interfaces and - on-the-wire protocols. Note that it would be possible - (albeit wasteful) to use ``bool`` in libvirt's logical wire - protocol, since XDR maps that to its lower-level ``bool_t`` - type, which **is** fixed-size. - - Don't compare a bool variable against the literal, ``true``, - since a value with a logical non-false value need not be - ``1``. I.e., don't write ``if (seen == true) ...``. Rather, - write ``if (seen)...``. +- While using ``bool`` is good for readability, it comes with a + minor caveat: Don't use ``bool`` in places where the type size + must be constant across all systems, like public interfaces and + on-the-wire protocols. Note that it would be possible (albeit + wasteful) to use ``bool`` in libvirt's logical wire protocol, + since XDR maps that to its lower-level ``bool_t`` type, which + **is** fixed-size. Of course, take all of the above with a grain of salt. If you're about to use some system interface that requires a type like -- 2.31.1
On a Friday in 2022, Tim Wiederhake wrote:
The code style showed `bool hasFoos; if (hasFoos == true)` as a good example in one place, only to warn against comparisons with `true` a couple of paragraphs further down.
Merge this advice on comparing with `true` into the "Conditional expressions" section and split the example up for readability.
Signed-off-by: Tim Wiederhake <twiederh@redhat.com> --- docs/coding-style.rst | 60 +++++++++++++++++++++++++++---------------- 1 file changed, 38 insertions(+), 22 deletions(-)
diff --git a/docs/coding-style.rst b/docs/coding-style.rst index dca9de1915..3dedb032f4 100644 --- a/docs/coding-style.rst +++ b/docs/coding-style.rst @@ -427,25 +427,47 @@ Conditional expressions -----------------------
For readability reasons new code should avoid shortening -comparisons to 0 for numeric types. Boolean and pointer -comparisons may be shortened. All long forms are okay: +comparisons to 0 for numeric types:
::
- virFoo *foos = NULL; size nfoos = 0; - bool hasFoos = false;
GOOD: - if (!foos) - if (!hasFoos) + if (nfoos != 0) if (nfoos == 0) - if (foos == NULL) - if (hasFoos == true)
BAD: - if (!nfoos) if (nfoos) + if (!nfoos) + +Prefer the shortened version for boolean values. Boolean values +should never be compared against the literal ``true``, as a +logical non-false value need not be ``1``. + +:: + + bool hasFoos = false; + + GOOD: + if (hasFoos) + if (!hasFoos) + + BAD: + if (hasFoos == true) + if (hasFoos != false) + if (hasFoos == false) + if (hasFoos != true)
I think people would get it even if all four options weren't listed :)
+ +Pointer comparisons may be shortened. All long forms are okay. +
Either way: Reviewed-by: Ján Tomko <jtomko@redhat.com> Jano
There are no instances of that label left. Signed-off-by: Tim Wiederhake <twiederh@redhat.com> --- docs/coding-style.rst | 1 - 1 file changed, 1 deletion(-) diff --git a/docs/coding-style.rst b/docs/coding-style.rst index 3dedb032f4..14c5136398 100644 --- a/docs/coding-style.rst +++ b/docs/coding-style.rst @@ -953,7 +953,6 @@ makes sense: error: A path only taken upon return with an error code cleanup: A path taken upon return with success code + optional error - no_memory: A path only taken upon return with an OOM error code retry: If needing to jump upwards (e.g., retry on EINTR) Top-level labels should be indented by one space (putting them on -- 2.31.1
On a Friday in 2022, Tim Wiederhake wrote:
There are no instances of that label left.
Signed-off-by: Tim Wiederhake <twiederh@redhat.com> --- docs/coding-style.rst | 1 - 1 file changed, 1 deletion(-)
Reviewed-by: Ján Tomko <jtomko@redhat.com> Jano
This was not mentioned before. Signed-off-by: Tim Wiederhake <twiederh@redhat.com> --- docs/coding-style.rst | 13 +++++++++++++ 1 file changed, 13 insertions(+) diff --git a/docs/coding-style.rst b/docs/coding-style.rst index 14c5136398..e1ed34f764 100644 --- a/docs/coding-style.rst +++ b/docs/coding-style.rst @@ -600,6 +600,19 @@ calling another function. ... } +Define variables on separate lines. This allows for smaller, easier to +understand diffs when changing them. Define variables in the smallest +possible scope. + +:: + + GOOD: + int x; + int y; + + BAD: + int x, y; + Attribute annotations --------------------- -- 2.31.1
On a Friday in 2022, Tim Wiederhake wrote:
This was not mentioned before.
Signed-off-by: Tim Wiederhake <twiederh@redhat.com> --- docs/coding-style.rst | 13 +++++++++++++ 1 file changed, 13 insertions(+)
diff --git a/docs/coding-style.rst b/docs/coding-style.rst index 14c5136398..e1ed34f764 100644 --- a/docs/coding-style.rst +++ b/docs/coding-style.rst @@ -600,6 +600,19 @@ calling another function. ... }
+Define variables on separate lines. This allows for smaller, easier to +understand diffs when changing them. Define variables in the smallest +possible scope. + +:: + + GOOD: + int x; + int y; + + BAD: + int x, y; +
Please use longer variable names and initialize some too, to illustrate it better, e.g.: int count = 0, nnodes; Personally I don't mind: size_t i, j; that much - even though removing one does cause churn, they are simple to read. Reviewed-by: Ján Tomko <jtomko@redhat.com> Jano
On 1/14/22 10:56 AM, Ján Tomko wrote:
On a Friday in 2022, Tim Wiederhake wrote:
This was not mentioned before.
Signed-off-by: Tim Wiederhake <twiederh@redhat.com> --- docs/coding-style.rst | 13 +++++++++++++ 1 file changed, 13 insertions(+)
diff --git a/docs/coding-style.rst b/docs/coding-style.rst index 14c5136398..e1ed34f764 100644 --- a/docs/coding-style.rst +++ b/docs/coding-style.rst @@ -600,6 +600,19 @@ calling another function. ... }
+Define variables on separate lines. This allows for smaller, easier to +understand diffs when changing them. Define variables in the smallest +possible scope. + +:: + + GOOD: + int x; + int y; + + BAD: + int x, y; +
Please use longer variable names and initialize some too, to illustrate it better, e.g.:
int count = 0, nnodes;
Personally I don't mind:
size_t i, j;
that much - even though removing one does cause churn, they are simple to read.
I also don't mind combining simple things like that, but am willing to go full-isolated just for consistency's sake. Since it's Friday and we're talking about personal preferences - I personally dislike the use of i and j (and anything else with a single letter) as variable names, because it makes using a text search for occurences pointless. Sure, longer variable names could also be a substring of something else, and any variable could be re-used elsewhere, but even then a search is mildly usable. (On the other hand, sometimes a loop is just a loop and it takes too much brain capacity to think of a meaningful name for the index. I used to work with someone who always used "ii" and "jj" for generic loop indexes because they were then easy to search for with few false positives (well - "ascii", "skiing", and a surprisingly high number of other more obscure words, but still...) , and I internalized that practice myself. After having libvirt patches with that rejected a couple times, I unlearned and conformed to the hive :-))
On a Friday in 2022, Laine Stump wrote:
Since it's Friday and we're talking about personal preferences - I personally dislike the use of i and j (and anything else with a single letter) as variable names, because it makes using a text search for occurences pointless. Sure, longer variable names could also be a substring of something else, and any variable could be re-used elsewhere, but even then a search is mildly usable.
Well, you need to search for the word i instead of the letter i. grep has the '-w' switch for that, or you can specify some boundaries: \bi\b \<i\> vim searches for the word under the cursor with '*' by default Surely other search tools have some equivalent.
(On the other hand, sometimes a loop is just a loop and it takes too much brain capacity to think of a meaningful name for the index. I used to work with someone who always used "ii" and "jj" for generic loop indexes because they were then easy to search for with few false positives (well - "ascii", "skiing", and a surprisingly high number of other more obscure words, but still...) , and I internalized that practice myself. After having libvirt patches with that rejected a couple times, I unlearned and conformed to the hive :-))
II thank you. JJano
On 1/14/22 3:29 PM, Ján Tomko wrote:
On a Friday in 2022, Laine Stump wrote:
Since it's Friday and we're talking about personal preferences - I personally dislike the use of i and j (and anything else with a single letter) as variable names, because it makes using a text search for occurences pointless. Sure, longer variable names could also be a substring of something else, and any variable could be re-used elsewhere, but even then a search is mildly usable.
Well, you need to search for the word i instead of the letter i.
grep has the '-w' switch for that, or you can specify some boundaries: \bi\b \<i\>
vim searches for the word under the cursor with '*' by default
Surely other search tools have some equivalent.
This forced me to go look for it in emacs, and after 28 years, I've learned about isearch-forward-symbol-at-point, which is by default bound to [alt-s .]. But that's just another different keystroke I have to remember. Much easier if I can just use an expansion of the ctl-s (incremental search) that I already know and use for pretty much all searching within a single file.
(On the other hand, sometimes a loop is just a loop and it takes too much brain capacity to think of a meaningful name for the index. I used to work with someone who always used "ii" and "jj" for generic loop indexes because they were then easy to search for with few false positives (well - "ascii", "skiing", and a surprisingly high number of other more obscure words, but still...) , and I internalized that practice myself. After having libvirt patches with that rejected a couple times, I unlearned and conformed to the hive :-))
II thank you.
JJano
KKind of you, LLaine
On Sat, Jan 15, 2022 at 02:22 Laine Stump <laine@redhat.com> wrote:
On 1/14/22 3:29 PM, Ján Tomko wrote:
On a Friday in 2022, Laine Stump wrote:
Since it's Friday and we're talking about personal preferences - I personally dislike the use of i and j (and anything else with a single letter) as variable names, because it makes using a text search for occurences pointless. Sure, longer variable names could also be a substring of something else, and any variable could be re-used elsewhere, but even then a search is mildly usable.
Well, you need to search for the word i instead of the letter i.
grep has the '-w' switch for that, or you can specify some boundaries: \bi\b \<i\>
vim searches for the word under the cursor with '*' by default
Surely other search tools have some equivalent.
This forced me to go look for it in emacs, and after 28 years, I've learned about isearch-forward-symbol-at-point, which is by default bound to [alt-s .]. But that's just another different keystroke I have to remember. Much easier if I can just use an expansion of the ctl-s (incremental search) that I already know and use for pretty much all searching within a single file.
Haha ! I use emacs as well and I never knew about this too. Will try it too. Thanks!
participants (6)
-
Andrea Bolognani -
Ani Sinha -
Daniel P. Berrangé -
Ján Tomko -
Laine Stump -
Tim Wiederhake