[libvirt PATCH v2 0/2] 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. V1: https://listman.redhat.com/archives/libvir-list/2022-January/msg00631.html Changes since V1: * patch 1: Do create typedefs for public types, do not for private types * patches 2 and 3: Pushed * patch 4 (now 2): Rewritten examples as requested Tim Wiederhake (2): docs: coding-style: Clarify on virXXXPtr types docs: coding-style: One variable declaration per line docs/coding-style.rst | 20 +++++++++++++++++++- 1 file changed, 19 insertions(+), 1 deletion(-) -- 2.31.1
This partially reverts commit 9ccbed6afb. Signed-off-by: Tim Wiederhake <twiederh@redhat.com> --- docs/coding-style.rst | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/docs/coding-style.rst b/docs/coding-style.rst index 37e6009db4..ee4d551805 100644 --- a/docs/coding-style.rst +++ b/docs/coding-style.rst @@ -53,10 +53,15 @@ Struct type names All structs should have a 'vir' prefix in their typedef name, 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. + name with a leading underscore. For types that are part of the + public API, a second typedef should be given for a pointer to + the struct with a 'Ptr' suffix. Do not introduce new such + typedefs for internal types. + :: typedef struct _virHashTable virHashTable; + typedef virHashTable *virHashTablePtr; struct _virHashTable { ... }; -- 2.31.1
On Mon, Jan 17, 2022 at 11:19:02 +0100, Tim Wiederhake wrote:
This partially reverts commit 9ccbed6afb.
Signed-off-by: Tim Wiederhake <twiederh@redhat.com> --- docs/coding-style.rst | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-)
diff --git a/docs/coding-style.rst b/docs/coding-style.rst index 37e6009db4..ee4d551805 100644 --- a/docs/coding-style.rst +++ b/docs/coding-style.rst @@ -53,10 +53,15 @@ Struct type names All structs should have a 'vir' prefix in their typedef name, 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. + name with a leading underscore. For types that are part of the + public API, a second typedef should be given for a pointer to + the struct with a 'Ptr' suffix. Do not introduce new such + typedefs for internal types. + ::
typedef struct _virHashTable virHashTable; + typedef virHashTable *virHashTablePtr;
IMO this is wrong. 'virHashTable' is (or rather was) an internal type so the 'virHashTablePtr' version must not be defined for it. The example should make a clear distinction if you want to add the caveat about public types. Additionally virHashTable doesn't exist any more. I can fix that bit if you think it's out of scope of your patches since I'm the one who removed that. Ideally we use some fake types which won't run the risk of being refactored later.
struct _virHashTable { ... }; -- 2.31.1
On Mon, 2022-01-17 at 11:40 +0100, Peter Krempa wrote:
On Mon, Jan 17, 2022 at 11:19:02 +0100, Tim Wiederhake wrote:
This partially reverts commit 9ccbed6afb.
Signed-off-by: Tim Wiederhake <twiederh@redhat.com> --- docs/coding-style.rst | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-)
diff --git a/docs/coding-style.rst b/docs/coding-style.rst index 37e6009db4..ee4d551805 100644 --- a/docs/coding-style.rst +++ b/docs/coding-style.rst @@ -53,10 +53,15 @@ Struct type names All structs should have a 'vir' prefix in their typedef name, 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. + name with a leading underscore. For types that are part of the + public API, a second typedef should be given for a pointer to + the struct with a 'Ptr' suffix. Do not introduce new such + typedefs for internal types. + :: typedef struct _virHashTable virHashTable; + typedef virHashTable *virHashTablePtr;
IMO this is wrong. 'virHashTable' is (or rather was) an internal type so the 'virHashTablePtr' version must not be defined for it.
The example should make a clear distinction if you want to add the caveat about public types.
Additionally virHashTable doesn't exist any more. I can fix that bit if you think it's out of scope of your patches since I'm the one who removed that.
Ideally we use some fake types which won't run the risk of being refactored later.
Good point, virHashTablePtr is a poor example. Will replace with a fake type as suggested.
struct _virHashTable { ... }; -- 2.31.1
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 ee4d551805..01f1e25127 100644 --- a/docs/coding-style.rst +++ b/docs/coding-style.rst @@ -600,6 +600,19 @@ calling another function. ... } +Prefer variable definitions on separate lines. This allows for smaller, +easier to understand diffs when changing them. Define variables in the +smallest possible scope. + +:: + + GOOD: + int count = 0; + int nnodes; + + BAD: + int count = 0, nnodes; + Attribute annotations --------------------- -- 2.31.1
participants (2)
-
Peter Krempa -
Tim Wiederhake