* [Qemu-devel] [PATCH v2 0/2] spapr_pci: coding style fixes
@ 2018-10-12 9:04 Greg Kurz
2018-10-12 9:05 ` [Qemu-devel] [PATCH v2 1/2] spapr_pci: convert g_malloc() to g_new() Greg Kurz
2018-10-12 9:05 ` [Qemu-devel] [PATCH v2 2/2] spapr_pci: rename some structured types Greg Kurz
0 siblings, 2 replies; 8+ messages in thread
From: Greg Kurz @ 2018-10-12 9:04 UTC (permalink / raw)
To: qemu-devel; +Cc: qemu-ppc, David Gibson, Philippe Mathieu-Daudé
As requested by David, two distinct patches and PCI still mentionned
in the new type names.
--
Greg
---
Greg Kurz (2):
spapr_pci: convert g_malloc() to g_new()
spapr_pci: rename some structured types
hw/ppc/spapr_pci.c | 22 +++++++++++-----------
include/hw/pci-host/spapr.h | 12 ++++++------
2 files changed, 17 insertions(+), 17 deletions(-)
^ permalink raw reply [flat|nested] 8+ messages in thread
* [Qemu-devel] [PATCH v2 1/2] spapr_pci: convert g_malloc() to g_new()
2018-10-12 9:04 [Qemu-devel] [PATCH v2 0/2] spapr_pci: coding style fixes Greg Kurz
@ 2018-10-12 9:05 ` Greg Kurz
2018-10-12 9:43 ` Philippe Mathieu-Daudé
2018-10-15 1:04 ` David Gibson
2018-10-12 9:05 ` [Qemu-devel] [PATCH v2 2/2] spapr_pci: rename some structured types Greg Kurz
1 sibling, 2 replies; 8+ messages in thread
From: Greg Kurz @ 2018-10-12 9:05 UTC (permalink / raw)
To: qemu-devel; +Cc: qemu-ppc, David Gibson, Philippe Mathieu-Daudé
When allocating an array, it is a recommended coding practice to call
g_new(FooType, n) instead of g_malloc(n * sizeof(FooType)) because
it takes care to avoid overflow when calculating the size of the
allocated block and it returns FooType *, which allows the compiler
to perform type checking.
Signed-off-by: Greg Kurz <groug@kaod.org>
---
hw/ppc/spapr_pci.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/hw/ppc/spapr_pci.c b/hw/ppc/spapr_pci.c
index c2271e6ed462..0537ce018f51 100644
--- a/hw/ppc/spapr_pci.c
+++ b/hw/ppc/spapr_pci.c
@@ -1883,7 +1883,7 @@ static int spapr_pci_pre_save(void *opaque)
if (!sphb->msi_devs_num) {
return 0;
}
- sphb->msi_devs = g_malloc(sphb->msi_devs_num * sizeof(spapr_pci_msi_mig));
+ sphb->msi_devs = g_new(spapr_pci_msi_mig, sphb->msi_devs_num);
g_hash_table_iter_init(&iter, sphb->msi);
for (i = 0; g_hash_table_iter_next(&iter, &key, &value); ++i) {
^ permalink raw reply related [flat|nested] 8+ messages in thread
* [Qemu-devel] [PATCH v2 2/2] spapr_pci: rename some structured types
2018-10-12 9:04 [Qemu-devel] [PATCH v2 0/2] spapr_pci: coding style fixes Greg Kurz
2018-10-12 9:05 ` [Qemu-devel] [PATCH v2 1/2] spapr_pci: convert g_malloc() to g_new() Greg Kurz
@ 2018-10-12 9:05 ` Greg Kurz
2018-10-15 1:49 ` Alexey Kardashevskiy
1 sibling, 1 reply; 8+ messages in thread
From: Greg Kurz @ 2018-10-12 9:05 UTC (permalink / raw)
To: qemu-devel; +Cc: qemu-ppc, David Gibson, Philippe Mathieu-Daudé
According to CODING_STYLE, structured types names are expected to be
in CamelCase but we have:
typedef struct spapr_pci_msi {
uint32_t first_irq;
uint32_t num;
} spapr_pci_msi;
typedef struct spapr_pci_msi_mig {
uint32_t key;
spapr_pci_msi value;
} spapr_pci_msi_mig;
Acronyms are often written in upper-case, but here we would en up with
a lot of upper-case letters in a row (ie, sPAPRPCIMSI) which defeats the
purpose of CamelCase. It even displays "RPC" which looks awkward.
This patch twists the rule a bit to keep the type names readable:
sPAPRpciMSI and sPAPRpciMSImig.
Signed-off-by: Greg Kurz <groug@kaod.org>
---
v2: - moved g_malloc change to a separate patch
- new naming proposal that doesn't drop PCI
- more detailed description in the changelog
---
hw/ppc/spapr_pci.c | 22 +++++++++++-----------
include/hw/pci-host/spapr.h | 12 ++++++------
2 files changed, 17 insertions(+), 17 deletions(-)
diff --git a/hw/ppc/spapr_pci.c b/hw/ppc/spapr_pci.c
index 0537ce018f51..2ee933e2d1ec 100644
--- a/hw/ppc/spapr_pci.c
+++ b/hw/ppc/spapr_pci.c
@@ -277,7 +277,7 @@ static void rtas_ibm_change_msi(PowerPCCPU *cpu, sPAPRMachineState *spapr,
unsigned int irq, max_irqs = 0;
sPAPRPHBState *phb = NULL;
PCIDevice *pdev = NULL;
- spapr_pci_msi *msi;
+ sPAPRpciMSI *msi;
int *config_addr_key;
Error *err = NULL;
int i;
@@ -325,7 +325,7 @@ static void rtas_ibm_change_msi(PowerPCCPU *cpu, sPAPRMachineState *spapr,
return;
}
- msi = (spapr_pci_msi *) g_hash_table_lookup(phb->msi, &config_addr);
+ msi = (sPAPRpciMSI *) g_hash_table_lookup(phb->msi, &config_addr);
/* Releasing MSIs */
if (!req_num) {
@@ -414,7 +414,7 @@ static void rtas_ibm_change_msi(PowerPCCPU *cpu, sPAPRMachineState *spapr,
irq, req_num);
/* Add MSI device to cache */
- msi = g_new(spapr_pci_msi, 1);
+ msi = g_new(sPAPRpciMSI, 1);
msi->first_irq = irq;
msi->num = req_num;
config_addr_key = g_new(int, 1);
@@ -445,7 +445,7 @@ static void rtas_ibm_query_interrupt_source_number(PowerPCCPU *cpu,
unsigned int intr_src_num = -1, ioa_intr_num = rtas_ld(args, 3);
sPAPRPHBState *phb = NULL;
PCIDevice *pdev = NULL;
- spapr_pci_msi *msi;
+ sPAPRpciMSI *msi;
/* Find sPAPRPHBState */
phb = spapr_pci_find_phb(spapr, buid);
@@ -458,7 +458,7 @@ static void rtas_ibm_query_interrupt_source_number(PowerPCCPU *cpu,
}
/* Find device descriptor and start IRQ */
- msi = (spapr_pci_msi *) g_hash_table_lookup(phb->msi, &config_addr);
+ msi = (sPAPRpciMSI *) g_hash_table_lookup(phb->msi, &config_addr);
if (!msi || !msi->first_irq || !msi->num || (ioa_intr_num >= msi->num)) {
trace_spapr_pci_msi("Failed to return vector", config_addr);
rtas_st(rets, 0, RTAS_OUT_HW_ERROR);
@@ -1849,9 +1849,9 @@ static const VMStateDescription vmstate_spapr_pci_msi = {
.version_id = 1,
.minimum_version_id = 1,
.fields = (VMStateField []) {
- VMSTATE_UINT32(key, spapr_pci_msi_mig),
- VMSTATE_UINT32(value.first_irq, spapr_pci_msi_mig),
- VMSTATE_UINT32(value.num, spapr_pci_msi_mig),
+ VMSTATE_UINT32(key, sPAPRpciMSImig),
+ VMSTATE_UINT32(value.first_irq, sPAPRpciMSImig),
+ VMSTATE_UINT32(value.num, sPAPRpciMSImig),
VMSTATE_END_OF_LIST()
},
};
@@ -1883,12 +1883,12 @@ static int spapr_pci_pre_save(void *opaque)
if (!sphb->msi_devs_num) {
return 0;
}
- sphb->msi_devs = g_new(spapr_pci_msi_mig, sphb->msi_devs_num);
+ sphb->msi_devs = g_new(sPAPRpciMSImig, sphb->msi_devs_num);
g_hash_table_iter_init(&iter, sphb->msi);
for (i = 0; g_hash_table_iter_next(&iter, &key, &value); ++i) {
sphb->msi_devs[i].key = *(uint32_t *) key;
- sphb->msi_devs[i].value = *(spapr_pci_msi *) value;
+ sphb->msi_devs[i].value = *(sPAPRpciMSI *) value;
}
return 0;
@@ -1938,7 +1938,7 @@ static const VMStateDescription vmstate_spapr_pci = {
vmstate_spapr_pci_lsi, struct spapr_pci_lsi),
VMSTATE_INT32(msi_devs_num, sPAPRPHBState),
VMSTATE_STRUCT_VARRAY_ALLOC(msi_devs, sPAPRPHBState, msi_devs_num, 0,
- vmstate_spapr_pci_msi, spapr_pci_msi_mig),
+ vmstate_spapr_pci_msi, sPAPRpciMSImig),
VMSTATE_END_OF_LIST()
},
};
diff --git a/include/hw/pci-host/spapr.h b/include/hw/pci-host/spapr.h
index 7c66c3872f96..eb8436b4fc32 100644
--- a/include/hw/pci-host/spapr.h
+++ b/include/hw/pci-host/spapr.h
@@ -34,15 +34,15 @@
typedef struct sPAPRPHBState sPAPRPHBState;
-typedef struct spapr_pci_msi {
+typedef struct sPAPRpciMSI {
uint32_t first_irq;
uint32_t num;
-} spapr_pci_msi;
+} sPAPRpciMSI;
-typedef struct spapr_pci_msi_mig {
+typedef struct sPAPRpciMSImig {
uint32_t key;
- spapr_pci_msi value;
-} spapr_pci_msi_mig;
+ sPAPRpciMSI value;
+} sPAPRpciMSImig;
struct sPAPRPHBState {
PCIHostState parent_obj;
@@ -70,7 +70,7 @@ struct sPAPRPHBState {
GHashTable *msi;
/* Temporary cache for migration purposes */
int32_t msi_devs_num;
- spapr_pci_msi_mig *msi_devs;
+ sPAPRpciMSImig *msi_devs;
QLIST_ENTRY(sPAPRPHBState) list;
^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [Qemu-devel] [PATCH v2 1/2] spapr_pci: convert g_malloc() to g_new()
2018-10-12 9:05 ` [Qemu-devel] [PATCH v2 1/2] spapr_pci: convert g_malloc() to g_new() Greg Kurz
@ 2018-10-12 9:43 ` Philippe Mathieu-Daudé
2018-10-15 1:04 ` David Gibson
1 sibling, 0 replies; 8+ messages in thread
From: Philippe Mathieu-Daudé @ 2018-10-12 9:43 UTC (permalink / raw)
To: Greg Kurz, qemu-devel; +Cc: qemu-ppc, Philippe Mathieu-Daudé, David Gibson
On 12/10/2018 11:05, Greg Kurz wrote:
> When allocating an array, it is a recommended coding practice to call
> g_new(FooType, n) instead of g_malloc(n * sizeof(FooType)) because
> it takes care to avoid overflow when calculating the size of the
> allocated block and it returns FooType *, which allows the compiler
> to perform type checking.
>
> Signed-off-by: Greg Kurz <groug@kaod.org>
Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> ---
> hw/ppc/spapr_pci.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/hw/ppc/spapr_pci.c b/hw/ppc/spapr_pci.c
> index c2271e6ed462..0537ce018f51 100644
> --- a/hw/ppc/spapr_pci.c
> +++ b/hw/ppc/spapr_pci.c
> @@ -1883,7 +1883,7 @@ static int spapr_pci_pre_save(void *opaque)
> if (!sphb->msi_devs_num) {
> return 0;
> }
> - sphb->msi_devs = g_malloc(sphb->msi_devs_num * sizeof(spapr_pci_msi_mig));
> + sphb->msi_devs = g_new(spapr_pci_msi_mig, sphb->msi_devs_num);
>
> g_hash_table_iter_init(&iter, sphb->msi);
> for (i = 0; g_hash_table_iter_next(&iter, &key, &value); ++i) {
>
>
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [Qemu-devel] [PATCH v2 1/2] spapr_pci: convert g_malloc() to g_new()
2018-10-12 9:05 ` [Qemu-devel] [PATCH v2 1/2] spapr_pci: convert g_malloc() to g_new() Greg Kurz
2018-10-12 9:43 ` Philippe Mathieu-Daudé
@ 2018-10-15 1:04 ` David Gibson
1 sibling, 0 replies; 8+ messages in thread
From: David Gibson @ 2018-10-15 1:04 UTC (permalink / raw)
To: Greg Kurz; +Cc: qemu-devel, qemu-ppc, Philippe Mathieu-Daudé
[-- Attachment #1: Type: text/plain, Size: 1323 bytes --]
On Fri, Oct 12, 2018 at 11:05:09AM +0200, Greg Kurz wrote:
> When allocating an array, it is a recommended coding practice to call
> g_new(FooType, n) instead of g_malloc(n * sizeof(FooType)) because
> it takes care to avoid overflow when calculating the size of the
> allocated block and it returns FooType *, which allows the compiler
> to perform type checking.
>
> Signed-off-by: Greg Kurz <groug@kaod.org>
Applied to ppc-for-3.1, thanks.
> ---
> hw/ppc/spapr_pci.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/hw/ppc/spapr_pci.c b/hw/ppc/spapr_pci.c
> index c2271e6ed462..0537ce018f51 100644
> --- a/hw/ppc/spapr_pci.c
> +++ b/hw/ppc/spapr_pci.c
> @@ -1883,7 +1883,7 @@ static int spapr_pci_pre_save(void *opaque)
> if (!sphb->msi_devs_num) {
> return 0;
> }
> - sphb->msi_devs = g_malloc(sphb->msi_devs_num * sizeof(spapr_pci_msi_mig));
> + sphb->msi_devs = g_new(spapr_pci_msi_mig, sphb->msi_devs_num);
>
> g_hash_table_iter_init(&iter, sphb->msi);
> for (i = 0; g_hash_table_iter_next(&iter, &key, &value); ++i) {
>
--
David Gibson | I'll have my music baroque, and my code
david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_
| _way_ _around_!
http://www.ozlabs.org/~dgibson
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [Qemu-devel] [PATCH v2 2/2] spapr_pci: rename some structured types
2018-10-12 9:05 ` [Qemu-devel] [PATCH v2 2/2] spapr_pci: rename some structured types Greg Kurz
@ 2018-10-15 1:49 ` Alexey Kardashevskiy
2018-11-07 4:46 ` David Gibson
0 siblings, 1 reply; 8+ messages in thread
From: Alexey Kardashevskiy @ 2018-10-15 1:49 UTC (permalink / raw)
To: Greg Kurz, qemu-devel; +Cc: qemu-ppc, Philippe Mathieu-Daudé, David Gibson
On 12/10/2018 20:05, Greg Kurz wrote:
> According to CODING_STYLE, structured types names are expected to be
> in CamelCase but we have:
>
> typedef struct spapr_pci_msi {
> uint32_t first_irq;
> uint32_t num;
> } spapr_pci_msi;
>
> typedef struct spapr_pci_msi_mig {
> uint32_t key;
> spapr_pci_msi value;
> } spapr_pci_msi_mig;
>
> Acronyms are often written in upper-case, but here we would en up with
> a lot of upper-case letters in a row (ie, sPAPRPCIMSI) which defeats the
> purpose of CamelCase. It even displays "RPC" which looks awkward.
Yet more common than this. I vote for sPAPRPCIMSI as PCI is an acronym.
"pci" is small letters hurts my eyes :)
> This patch twists the rule a bit to keep the type names readable:
> sPAPRpciMSI and sPAPRpciMSImig.
>
> Signed-off-by: Greg Kurz <groug@kaod.org>
> ---
> v2: - moved g_malloc change to a separate patch
> - new naming proposal that doesn't drop PCI
> - more detailed description in the changelog
> ---
> hw/ppc/spapr_pci.c | 22 +++++++++++-----------
> include/hw/pci-host/spapr.h | 12 ++++++------
> 2 files changed, 17 insertions(+), 17 deletions(-)
>
> diff --git a/hw/ppc/spapr_pci.c b/hw/ppc/spapr_pci.c
> index 0537ce018f51..2ee933e2d1ec 100644
> --- a/hw/ppc/spapr_pci.c
> +++ b/hw/ppc/spapr_pci.c
> @@ -277,7 +277,7 @@ static void rtas_ibm_change_msi(PowerPCCPU *cpu, sPAPRMachineState *spapr,
> unsigned int irq, max_irqs = 0;
> sPAPRPHBState *phb = NULL;
> PCIDevice *pdev = NULL;
> - spapr_pci_msi *msi;
> + sPAPRpciMSI *msi;
> int *config_addr_key;
> Error *err = NULL;
> int i;
> @@ -325,7 +325,7 @@ static void rtas_ibm_change_msi(PowerPCCPU *cpu, sPAPRMachineState *spapr,
> return;
> }
>
> - msi = (spapr_pci_msi *) g_hash_table_lookup(phb->msi, &config_addr);
> + msi = (sPAPRpciMSI *) g_hash_table_lookup(phb->msi, &config_addr);
>
> /* Releasing MSIs */
> if (!req_num) {
> @@ -414,7 +414,7 @@ static void rtas_ibm_change_msi(PowerPCCPU *cpu, sPAPRMachineState *spapr,
> irq, req_num);
>
> /* Add MSI device to cache */
> - msi = g_new(spapr_pci_msi, 1);
> + msi = g_new(sPAPRpciMSI, 1);
> msi->first_irq = irq;
> msi->num = req_num;
> config_addr_key = g_new(int, 1);
> @@ -445,7 +445,7 @@ static void rtas_ibm_query_interrupt_source_number(PowerPCCPU *cpu,
> unsigned int intr_src_num = -1, ioa_intr_num = rtas_ld(args, 3);
> sPAPRPHBState *phb = NULL;
> PCIDevice *pdev = NULL;
> - spapr_pci_msi *msi;
> + sPAPRpciMSI *msi;
>
> /* Find sPAPRPHBState */
> phb = spapr_pci_find_phb(spapr, buid);
> @@ -458,7 +458,7 @@ static void rtas_ibm_query_interrupt_source_number(PowerPCCPU *cpu,
> }
>
> /* Find device descriptor and start IRQ */
> - msi = (spapr_pci_msi *) g_hash_table_lookup(phb->msi, &config_addr);
> + msi = (sPAPRpciMSI *) g_hash_table_lookup(phb->msi, &config_addr);
> if (!msi || !msi->first_irq || !msi->num || (ioa_intr_num >= msi->num)) {
> trace_spapr_pci_msi("Failed to return vector", config_addr);
> rtas_st(rets, 0, RTAS_OUT_HW_ERROR);
> @@ -1849,9 +1849,9 @@ static const VMStateDescription vmstate_spapr_pci_msi = {
> .version_id = 1,
> .minimum_version_id = 1,
> .fields = (VMStateField []) {
> - VMSTATE_UINT32(key, spapr_pci_msi_mig),
> - VMSTATE_UINT32(value.first_irq, spapr_pci_msi_mig),
> - VMSTATE_UINT32(value.num, spapr_pci_msi_mig),
> + VMSTATE_UINT32(key, sPAPRpciMSImig),
> + VMSTATE_UINT32(value.first_irq, sPAPRpciMSImig),
> + VMSTATE_UINT32(value.num, sPAPRpciMSImig),
> VMSTATE_END_OF_LIST()
> },
> };
> @@ -1883,12 +1883,12 @@ static int spapr_pci_pre_save(void *opaque)
> if (!sphb->msi_devs_num) {
> return 0;
> }
> - sphb->msi_devs = g_new(spapr_pci_msi_mig, sphb->msi_devs_num);
> + sphb->msi_devs = g_new(sPAPRpciMSImig, sphb->msi_devs_num);
>
> g_hash_table_iter_init(&iter, sphb->msi);
> for (i = 0; g_hash_table_iter_next(&iter, &key, &value); ++i) {
> sphb->msi_devs[i].key = *(uint32_t *) key;
> - sphb->msi_devs[i].value = *(spapr_pci_msi *) value;
> + sphb->msi_devs[i].value = *(sPAPRpciMSI *) value;
> }
>
> return 0;
> @@ -1938,7 +1938,7 @@ static const VMStateDescription vmstate_spapr_pci = {
> vmstate_spapr_pci_lsi, struct spapr_pci_lsi),
> VMSTATE_INT32(msi_devs_num, sPAPRPHBState),
> VMSTATE_STRUCT_VARRAY_ALLOC(msi_devs, sPAPRPHBState, msi_devs_num, 0,
> - vmstate_spapr_pci_msi, spapr_pci_msi_mig),
> + vmstate_spapr_pci_msi, sPAPRpciMSImig),
> VMSTATE_END_OF_LIST()
> },
> };
> diff --git a/include/hw/pci-host/spapr.h b/include/hw/pci-host/spapr.h
> index 7c66c3872f96..eb8436b4fc32 100644
> --- a/include/hw/pci-host/spapr.h
> +++ b/include/hw/pci-host/spapr.h
> @@ -34,15 +34,15 @@
>
> typedef struct sPAPRPHBState sPAPRPHBState;
>
> -typedef struct spapr_pci_msi {
> +typedef struct sPAPRpciMSI {
> uint32_t first_irq;
> uint32_t num;
> -} spapr_pci_msi;
> +} sPAPRpciMSI;
>
> -typedef struct spapr_pci_msi_mig {
> +typedef struct sPAPRpciMSImig {
> uint32_t key;
> - spapr_pci_msi value;
> -} spapr_pci_msi_mig;
> + sPAPRpciMSI value;
> +} sPAPRpciMSImig;
>
> struct sPAPRPHBState {
> PCIHostState parent_obj;
> @@ -70,7 +70,7 @@ struct sPAPRPHBState {
> GHashTable *msi;
> /* Temporary cache for migration purposes */
> int32_t msi_devs_num;
> - spapr_pci_msi_mig *msi_devs;
> + sPAPRpciMSImig *msi_devs;
>
> QLIST_ENTRY(sPAPRPHBState) list;
>
>
>
--
Alexey
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [Qemu-devel] [PATCH v2 2/2] spapr_pci: rename some structured types
2018-10-15 1:49 ` Alexey Kardashevskiy
@ 2018-11-07 4:46 ` David Gibson
2018-11-08 11:48 ` Greg Kurz
0 siblings, 1 reply; 8+ messages in thread
From: David Gibson @ 2018-11-07 4:46 UTC (permalink / raw)
To: Alexey Kardashevskiy
Cc: Greg Kurz, qemu-devel, qemu-ppc, Philippe Mathieu-Daudé
[-- Attachment #1: Type: text/plain, Size: 1597 bytes --]
On Mon, Oct 15, 2018 at 12:49:53PM +1100, Alexey Kardashevskiy wrote:
>
>
> On 12/10/2018 20:05, Greg Kurz wrote:
> > According to CODING_STYLE, structured types names are expected to be
> > in CamelCase but we have:
> >
> > typedef struct spapr_pci_msi {
> > uint32_t first_irq;
> > uint32_t num;
> > } spapr_pci_msi;
> >
> > typedef struct spapr_pci_msi_mig {
> > uint32_t key;
> > spapr_pci_msi value;
> > } spapr_pci_msi_mig;
> >
> > Acronyms are often written in upper-case, but here we would en up with
> > a lot of upper-case letters in a row (ie, sPAPRPCIMSI) which defeats the
> > purpose of CamelCase. It even displays "RPC" which looks awkward.
>
> Yet more common than this. I vote for sPAPRPCIMSI as PCI is an acronym.
> "pci" is small letters hurts my eyes :)
Hrm. So, yes, I know I kind of started it, but these various
compromises about how to captialize things means this patch is now
"change from non-camelcase to.. something that's also not really
camelcase".
At which point I'm not particularly convinced it's worth the bother.
If we really want to go ahead with CamelCasing this, I think we'd need
to start by fixing up the existing sorta-camelcase-but-not-really
stuff to actual camelcase. Which would mean the slightly odd reading
"Spapr" and "Pci" and "Msi" and so forth, but it might be worth it for
consistency.
--
David Gibson | I'll have my music baroque, and my code
david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_
| _way_ _around_!
http://www.ozlabs.org/~dgibson
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [Qemu-devel] [PATCH v2 2/2] spapr_pci: rename some structured types
2018-11-07 4:46 ` David Gibson
@ 2018-11-08 11:48 ` Greg Kurz
0 siblings, 0 replies; 8+ messages in thread
From: Greg Kurz @ 2018-11-08 11:48 UTC (permalink / raw)
To: David Gibson
Cc: Alexey Kardashevskiy, qemu-devel, qemu-ppc, Philippe Mathieu-Daudé
[-- Attachment #1: Type: text/plain, Size: 1926 bytes --]
On Wed, 7 Nov 2018 15:46:35 +1100
David Gibson <david@gibson.dropbear.id.au> wrote:
> On Mon, Oct 15, 2018 at 12:49:53PM +1100, Alexey Kardashevskiy wrote:
> >
> >
> > On 12/10/2018 20:05, Greg Kurz wrote:
> > > According to CODING_STYLE, structured types names are expected to be
> > > in CamelCase but we have:
> > >
> > > typedef struct spapr_pci_msi {
> > > uint32_t first_irq;
> > > uint32_t num;
> > > } spapr_pci_msi;
> > >
> > > typedef struct spapr_pci_msi_mig {
> > > uint32_t key;
> > > spapr_pci_msi value;
> > > } spapr_pci_msi_mig;
> > >
> > > Acronyms are often written in upper-case, but here we would en up with
> > > a lot of upper-case letters in a row (ie, sPAPRPCIMSI) which defeats the
> > > purpose of CamelCase. It even displays "RPC" which looks awkward.
> >
> > Yet more common than this. I vote for sPAPRPCIMSI as PCI is an acronym.
> > "pci" is small letters hurts my eyes :)
>
> Hrm. So, yes, I know I kind of started it, but these various
> compromises about how to captialize things means this patch is now
> "change from non-camelcase to.. something that's also not really
> camelcase".
>
> At which point I'm not particularly convinced it's worth the bother.
>
> If we really want to go ahead with CamelCasing this, I think we'd need
> to start by fixing up the existing sorta-camelcase-but-not-really
> stuff to actual camelcase. Which would mean the slightly odd reading
> "Spapr" and "Pci" and "Msi" and so forth, but it might be worth it for
> consistency.
>
Looking at sPAPR only we already get:
$ git grep sPAPR | wc -l
1070
I understand your point but this would cause a lot of changes,
ie, a lot of noise in git blame and probably harder backports
of subsequent commits... I guess it isn't worth the pain.
Maybe we can just forget this patch and live with this minor
coding style violation. :)
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2018-11-08 11:49 UTC | newest]
Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-10-12 9:04 [Qemu-devel] [PATCH v2 0/2] spapr_pci: coding style fixes Greg Kurz
2018-10-12 9:05 ` [Qemu-devel] [PATCH v2 1/2] spapr_pci: convert g_malloc() to g_new() Greg Kurz
2018-10-12 9:43 ` Philippe Mathieu-Daudé
2018-10-15 1:04 ` David Gibson
2018-10-12 9:05 ` [Qemu-devel] [PATCH v2 2/2] spapr_pci: rename some structured types Greg Kurz
2018-10-15 1:49 ` Alexey Kardashevskiy
2018-11-07 4:46 ` David Gibson
2018-11-08 11:48 ` Greg Kurz
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.