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;
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
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
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. :)
© 2016 - 2025 Red Hat, Inc.