[PATCH 00/26] constify local structures

[PATCH 00/26] constify local structures

[Julia Lawall]

Constify local structures.

The semantic patch that makes this change is as follows:
(http://coccinelle.lip6.fr/)

// <smpl>
// The first rule ignores some cases that posed problems
@r disable optional_qualifier@
identifier s != {peri_clk_data,threshold_attr,tracer_flags,tracer};
identifier i != {s5k5baf_cis_rect,smtcfb_fix};
position p;
@@
static struct s i@p = { ... };

@lstruct@
identifier r.s;
@@
struct s { ... };

@used depends on lstruct@
identifier r.i;
@@
i

@bad1@
expression e;
identifier r.i;
assignment operator a;
@@
 (<+...i...+>) a e

@bad2@
identifier r.i;
@@
 &(<+...i...+>)

@bad3@
identifier r.i;
declarer d;
@@
 d(...,<+...i...+>,...);

@bad4@
identifier r.i;
type T;
T[] e;
identifier f;
position p;
@@

f@p(...,
(
  (<+...i...+>)
&
  e
)
,...)

@bad4a@
identifier r.i;
type T;
T *e;
identifier f;
position p;
@@

f@p(...,
(
  (<+...i...+>)
&
  e
)
,...)

@ok5@
expression *e;
identifier r.i;
position p;
@@
e =@p i

@bad5@
expression *e;
identifier r.i;
position p != ok5.p;
@@
e =@p (<+...i...+>)

@rr depends on used && !bad1 && !bad2 && !bad3 && !bad4 && !bad4a && !bad5@
identifier s,r.i;
position r.p;
@@

static
+const
 struct s i@p = { ... };

@depends on used && !bad1 && !bad2 && !bad3 && !bad4 && !bad4a && !bad5
 disable optional_qualifier@
identifier rr.s,r.i;
@@

static
+const
 struct s i;
// </smpl>

---

 drivers/acpi/acpi_apd.c                              |    8 +++---
 drivers/char/tpm/tpm-interface.c                     |   10 ++++----
 drivers/char/tpm/tpm-sysfs.c                         |    2 -
 drivers/cpufreq/intel_pstate.c                       |    8 +++---
 drivers/infiniband/hw/i40iw/i40iw_uk.c               |    6 ++---
 drivers/media/i2c/tvp514x.c                          |    2 -
 drivers/media/pci/ddbridge/ddbridge-core.c           |   18 +++++++--------
 drivers/media/pci/ngene/ngene-cards.c                |   14 ++++++------
 drivers/media/pci/smipcie/smipcie-main.c             |    8 +++---
 drivers/misc/sgi-xp/xpc_uv.c                         |    2 -
 drivers/net/arcnet/com20020-pci.c                    |   10 ++++----
 drivers/net/can/c_can/c_can_pci.c                    |    4 +--
 drivers/net/can/sja1000/plx_pci.c                    |   20 ++++++++---------
 drivers/net/ethernet/mellanox/mlx4/main.c            |    4 +--
 drivers/net/ethernet/oki-semi/pch_gbe/pch_gbe_main.c |    2 -
 drivers/net/ethernet/renesas/sh_eth.c                |   14 ++++++------
 drivers/net/ethernet/stmicro/stmmac/stmmac_pci.c     |    2 -
 drivers/net/wireless/ath/dfs_pattern_detector.c      |    2 -
 drivers/net/wireless/intel/iwlegacy/3945.c           |    4 +--
 drivers/net/wireless/realtek/rtlwifi/rtl8188ee/sw.c  |    2 -
 drivers/net/wireless/realtek/rtlwifi/rtl8192ce/sw.c  |    2 -
 drivers/net/wireless/realtek/rtlwifi/rtl8192de/sw.c  |    2 -
 drivers/net/wireless/realtek/rtlwifi/rtl8192ee/sw.c  |    2 -
 drivers/net/wireless/realtek/rtlwifi/rtl8192se/sw.c  |    2 -
 drivers/net/wireless/realtek/rtlwifi/rtl8723ae/sw.c  |    2 -
 drivers/net/wireless/realtek/rtlwifi/rtl8723be/sw.c  |    2 -
 drivers/net/wireless/realtek/rtlwifi/rtl8821ae/sw.c  |    2 -
 drivers/platform/chrome/chromeos_laptop.c            |   22 +++++++++----------
 drivers/platform/x86/intel_scu_ipc.c                 |    6 ++---
 drivers/platform/x86/intel_telemetry_debugfs.c       |    2 -
 drivers/scsi/esas2r/esas2r_flash.c                   |    2 -
 drivers/scsi/hptiop.c                                |    6 ++---
 drivers/spi/spi-dw-pci.c                             |    4 +--
 drivers/staging/rtl8192e/rtl8192e/rtl_core.c         |    2 -
 drivers/usb/misc/ezusb.c                             |    2 -
 drivers/video/fbdev/matrox/matroxfb_g450.c           |    2 -
 lib/crc64_ecma.c                                     |    2 -
 sound/pci/ctxfi/ctatc.c                              |    2 -
 sound/pci/hda/patch_ca0132.c                         |   10 ++++----
 sound/pci/riptide/riptide.c                          |    2 -
 40 files changed, 110 insertions(+), 110 deletions(-)

[PATCH 13/26] [media]: constify local structures

[Julia Lawall]

For structure types defined in the same file or local header files, find
top-level static structure declarations that have the following
properties:
1. Never reassigned.
2. Address never taken
3. Not passed to a top-level macro call
4. No pointer or array-typed field passed to a function or stored in a
variable.
Declare structures having all of these properties as const.

Done using Coccinelle.
Based on a suggestion by Joe Perches <joe@perches.com>.

Signed-off-by: Julia Lawall <Julia.Lawall@lip6.fr>

---
The semantic patch seems too long for a commit log, but is in the cover
letter.

 drivers/media/i2c/tvp514x.c                |    2 +-
 drivers/media/pci/ddbridge/ddbridge-core.c |   18 +++++++++---------
 drivers/media/pci/ngene/ngene-cards.c      |   14 +++++++-------
 drivers/media/pci/smipcie/smipcie-main.c   |    8 ++++----
 4 files changed, 21 insertions(+), 21 deletions(-)

diff --git a/drivers/media/i2c/tvp514x.c b/drivers/media/i2c/tvp514x.c
index 7cdd948..43a9252 100644
--- a/drivers/media/i2c/tvp514x.c
+++ b/drivers/media/i2c/tvp514x.c
@@ -977,7 +977,7 @@ static const struct v4l2_subdev_ops tvp514x_ops = {
 	.pad = &tvp514x_pad_ops,
 };
 
-static struct tvp514x_decoder tvp514x_dev = {
+static const struct tvp514x_decoder tvp514x_dev = {
 	.streaming = 0,
 	.fmt_list = tvp514x_fmt_list,
 	.num_fmts = ARRAY_SIZE(tvp514x_fmt_list),
diff --git a/drivers/media/pci/ddbridge/ddbridge-core.c b/drivers/media/pci/ddbridge/ddbridge-core.c
index 47def73..18e3a4d 100644
--- a/drivers/media/pci/ddbridge/ddbridge-core.c
+++ b/drivers/media/pci/ddbridge/ddbridge-core.c
@@ -1643,53 +1643,53 @@ fail:
 /******************************************************************************/
 /******************************************************************************/
 
-static struct ddb_info ddb_none = {
+static const struct ddb_info ddb_none = {
 	.type     = DDB_NONE,
 	.name     = "Digital Devices PCIe bridge",
 };
 
-static struct ddb_info ddb_octopus = {
+static const struct ddb_info ddb_octopus = {
 	.type     = DDB_OCTOPUS,
 	.name     = "Digital Devices Octopus DVB adapter",
 	.port_num = 4,
 };
 
-static struct ddb_info ddb_octopus_le = {
+static const struct ddb_info ddb_octopus_le = {
 	.type     = DDB_OCTOPUS,
 	.name     = "Digital Devices Octopus LE DVB adapter",
 	.port_num = 2,
 };
 
-static struct ddb_info ddb_octopus_mini = {
+static const struct ddb_info ddb_octopus_mini = {
 	.type     = DDB_OCTOPUS,
 	.name     = "Digital Devices Octopus Mini",
 	.port_num = 4,
 };
 
-static struct ddb_info ddb_v6 = {
+static const struct ddb_info ddb_v6 = {
 	.type     = DDB_OCTOPUS,
 	.name     = "Digital Devices Cine S2 V6 DVB adapter",
 	.port_num = 3,
 };
-static struct ddb_info ddb_v6_5 = {
+static const struct ddb_info ddb_v6_5 = {
 	.type     = DDB_OCTOPUS,
 	.name     = "Digital Devices Cine S2 V6.5 DVB adapter",
 	.port_num = 4,
 };
 
-static struct ddb_info ddb_dvbct = {
+static const struct ddb_info ddb_dvbct = {
 	.type     = DDB_OCTOPUS,
 	.name     = "Digital Devices DVBCT V6.1 DVB adapter",
 	.port_num = 3,
 };
 
-static struct ddb_info ddb_satixS2v3 = {
+static const struct ddb_info ddb_satixS2v3 = {
 	.type     = DDB_OCTOPUS,
 	.name     = "Mystique SaTiX-S2 V3 DVB adapter",
 	.port_num = 3,
 };
 
-static struct ddb_info ddb_octopusv3 = {
+static const struct ddb_info ddb_octopusv3 = {
 	.type     = DDB_OCTOPUS,
 	.name     = "Digital Devices Octopus V3 DVB adapter",
 	.port_num = 4,
diff --git a/drivers/media/pci/ngene/ngene-cards.c b/drivers/media/pci/ngene/ngene-cards.c
index 4e783a6..423e8c8 100644
--- a/drivers/media/pci/ngene/ngene-cards.c
+++ b/drivers/media/pci/ngene/ngene-cards.c
@@ -613,7 +613,7 @@ static struct stv6110x_config tuner_cineS2_1 = {
 	.clk_div = 1,
 };
 
-static struct ngene_info ngene_info_cineS2 = {
+static const struct ngene_info ngene_info_cineS2 = {
 	.type		= NGENE_SIDEWINDER,
 	.name		= "Linux4Media cineS2 DVB-S2 Twin Tuner",
 	.io_type	= {NGENE_IO_TSIN, NGENE_IO_TSIN},
@@ -627,7 +627,7 @@ static struct ngene_info ngene_info_cineS2 = {
 	.msi_supported	= true,
 };
 
-static struct ngene_info ngene_info_satixS2 = {
+static const struct ngene_info ngene_info_satixS2 = {
 	.type		= NGENE_SIDEWINDER,
 	.name		= "Mystique SaTiX-S2 Dual",
 	.io_type	= {NGENE_IO_TSIN, NGENE_IO_TSIN},
@@ -641,7 +641,7 @@ static struct ngene_info ngene_info_satixS2 = {
 	.msi_supported	= true,
 };
 
-static struct ngene_info ngene_info_satixS2v2 = {
+static const struct ngene_info ngene_info_satixS2v2 = {
 	.type		= NGENE_SIDEWINDER,
 	.name		= "Mystique SaTiX-S2 Dual (v2)",
 	.io_type	= {NGENE_IO_TSIN, NGENE_IO_TSIN, NGENE_IO_TSIN, NGENE_IO_TSIN,
@@ -656,7 +656,7 @@ static struct ngene_info ngene_info_satixS2v2 = {
 	.msi_supported	= true,
 };
 
-static struct ngene_info ngene_info_cineS2v5 = {
+static const struct ngene_info ngene_info_cineS2v5 = {
 	.type		= NGENE_SIDEWINDER,
 	.name		= "Linux4Media cineS2 DVB-S2 Twin Tuner (v5)",
 	.io_type	= {NGENE_IO_TSIN, NGENE_IO_TSIN, NGENE_IO_TSIN, NGENE_IO_TSIN,
@@ -672,7 +672,7 @@ static struct ngene_info ngene_info_cineS2v5 = {
 };
 
 
-static struct ngene_info ngene_info_duoFlex = {
+static const struct ngene_info ngene_info_duoFlex = {
 	.type           = NGENE_SIDEWINDER,
 	.name           = "Digital Devices DuoFlex PCIe or miniPCIe",
 	.io_type        = {NGENE_IO_TSIN, NGENE_IO_TSIN, NGENE_IO_TSIN, NGENE_IO_TSIN,
@@ -687,7 +687,7 @@ static struct ngene_info ngene_info_duoFlex = {
 	.msi_supported	= true,
 };
 
-static struct ngene_info ngene_info_m780 = {
+static const struct ngene_info ngene_info_m780 = {
 	.type           = NGENE_APP,
 	.name           = "Aver M780 ATSC/QAM-B",
 
@@ -727,7 +727,7 @@ static struct drxd_config fe_terratec_dvbt_1 = {
 	.osc_deviation  = osc_deviation,
 };
 
-static struct ngene_info ngene_info_terratec = {
+static const struct ngene_info ngene_info_terratec = {
 	.type           = NGENE_TERRATEC,
 	.name           = "Terratec Integra/Cinergy2400i Dual DVB-T",
 	.io_type        = {NGENE_IO_TSIN, NGENE_IO_TSIN},
diff --git a/drivers/media/pci/smipcie/smipcie-main.c b/drivers/media/pci/smipcie/smipcie-main.c
index 83981d61..6dbe3b4 100644
--- a/drivers/media/pci/smipcie/smipcie-main.c
+++ b/drivers/media/pci/smipcie/smipcie-main.c
@@ -1060,7 +1060,7 @@ static void smi_remove(struct pci_dev *pdev)
 }
 
 /* DVBSky cards */
-static struct smi_cfg_info dvbsky_s950_cfg = {
+static const struct smi_cfg_info dvbsky_s950_cfg = {
 	.type = SMI_DVBSKY_S950,
 	.name = "DVBSky S950 V3",
 	.ts_0 = SMI_TS_NULL,
@@ -1070,7 +1070,7 @@ static struct smi_cfg_info dvbsky_s950_cfg = {
 	.rc_map = RC_MAP_DVBSKY,
 };
 
-static struct smi_cfg_info dvbsky_s952_cfg = {
+static const struct smi_cfg_info dvbsky_s952_cfg = {
 	.type = SMI_DVBSKY_S952,
 	.name = "DVBSky S952 V3",
 	.ts_0 = SMI_TS_DMA_BOTH,
@@ -1080,7 +1080,7 @@ static struct smi_cfg_info dvbsky_s952_cfg = {
 	.rc_map = RC_MAP_DVBSKY,
 };
 
-static struct smi_cfg_info dvbsky_t9580_cfg = {
+static const struct smi_cfg_info dvbsky_t9580_cfg = {
 	.type = SMI_DVBSKY_T9580,
 	.name = "DVBSky T9580 V3",
 	.ts_0 = SMI_TS_DMA_BOTH,
@@ -1090,7 +1090,7 @@ static struct smi_cfg_info dvbsky_t9580_cfg = {
 	.rc_map = RC_MAP_DVBSKY,
 };
 
-static struct smi_cfg_info technotrend_s2_4200_cfg = {
+static const struct smi_cfg_info technotrend_s2_4200_cfg = {
 	.type = SMI_TECHNOTREND_S2_4200,
 	.name = "TechnoTrend TT-budget S2-4200 Twin",
 	.ts_0 = SMI_TS_DMA_BOTH,

Re: [PATCH 00/26] constify local structures

[Jarkko Sakkinen]

On Sun, Sep 11, 2016 at 03:05:42PM +0200, Julia Lawall wrote:
> Constify local structures.
> 
> The semantic patch that makes this change is as follows:
> (http://coccinelle.lip6.fr/)

Just my two cents but:

1. You *can* use a static analysis too to find bugs or other issues.
2. However, you should manually do the commits and proper commit
   messages to subsystems based on your findings. And I generally think
   that if one contributes code one should also at least smoke test changes
   somehow.

I don't know if I'm alone with my opinion. I just think that one should
also do the analysis part and not blindly create and submit patches.

Anyway, I'll apply the TPM change at some point. As I said they were
for better. Thanks.

/Jarkko

> // <smpl>
> // The first rule ignores some cases that posed problems
> @r disable optional_qualifier@
> identifier s != {peri_clk_data,threshold_attr,tracer_flags,tracer};
> identifier i != {s5k5baf_cis_rect,smtcfb_fix};
> position p;
> @@
> static struct s i@p = { ... };
> 
> @lstruct@
> identifier r.s;
> @@
> struct s { ... };
> 
> @used depends on lstruct@
> identifier r.i;
> @@
> i
> 
> @bad1@
> expression e;
> identifier r.i;
> assignment operator a;
> @@
>  (<+...i...+>) a e
> 
> @bad2@
> identifier r.i;
> @@
>  &(<+...i...+>)
> 
> @bad3@
> identifier r.i;
> declarer d;
> @@
>  d(...,<+...i...+>,...);
> 
> @bad4@
> identifier r.i;
> type T;
> T[] e;
> identifier f;
> position p;
> @@
> 
> f@p(...,
> (
>   (<+...i...+>)
> &
>   e
> )
> ,...)
> 
> @bad4a@
> identifier r.i;
> type T;
> T *e;
> identifier f;
> position p;
> @@
> 
> f@p(...,
> (
>   (<+...i...+>)
> &
>   e
> )
> ,...)
> 
> @ok5@
> expression *e;
> identifier r.i;
> position p;
> @@
> e =@p i
> 
> @bad5@
> expression *e;
> identifier r.i;
> position p != ok5.p;
> @@
> e =@p (<+...i...+>)
> 
> @rr depends on used && !bad1 && !bad2 && !bad3 && !bad4 && !bad4a && !bad5@
> identifier s,r.i;
> position r.p;
> @@
> 
> static
> +const
>  struct s i@p = { ... };
> 
> @depends on used && !bad1 && !bad2 && !bad3 && !bad4 && !bad4a && !bad5
>  disable optional_qualifier@
> identifier rr.s,r.i;
> @@
> 
> static
> +const
>  struct s i;
> // </smpl>
> 
> ---
> 
>  drivers/acpi/acpi_apd.c                              |    8 +++---
>  drivers/char/tpm/tpm-interface.c                     |   10 ++++----
>  drivers/char/tpm/tpm-sysfs.c                         |    2 -
>  drivers/cpufreq/intel_pstate.c                       |    8 +++---
>  drivers/infiniband/hw/i40iw/i40iw_uk.c               |    6 ++---
>  drivers/media/i2c/tvp514x.c                          |    2 -
>  drivers/media/pci/ddbridge/ddbridge-core.c           |   18 +++++++--------
>  drivers/media/pci/ngene/ngene-cards.c                |   14 ++++++------
>  drivers/media/pci/smipcie/smipcie-main.c             |    8 +++---
>  drivers/misc/sgi-xp/xpc_uv.c                         |    2 -
>  drivers/net/arcnet/com20020-pci.c                    |   10 ++++----
>  drivers/net/can/c_can/c_can_pci.c                    |    4 +--
>  drivers/net/can/sja1000/plx_pci.c                    |   20 ++++++++---------
>  drivers/net/ethernet/mellanox/mlx4/main.c            |    4 +--
>  drivers/net/ethernet/oki-semi/pch_gbe/pch_gbe_main.c |    2 -
>  drivers/net/ethernet/renesas/sh_eth.c                |   14 ++++++------
>  drivers/net/ethernet/stmicro/stmmac/stmmac_pci.c     |    2 -
>  drivers/net/wireless/ath/dfs_pattern_detector.c      |    2 -
>  drivers/net/wireless/intel/iwlegacy/3945.c           |    4 +--
>  drivers/net/wireless/realtek/rtlwifi/rtl8188ee/sw.c  |    2 -
>  drivers/net/wireless/realtek/rtlwifi/rtl8192ce/sw.c  |    2 -
>  drivers/net/wireless/realtek/rtlwifi/rtl8192de/sw.c  |    2 -
>  drivers/net/wireless/realtek/rtlwifi/rtl8192ee/sw.c  |    2 -
>  drivers/net/wireless/realtek/rtlwifi/rtl8192se/sw.c  |    2 -
>  drivers/net/wireless/realtek/rtlwifi/rtl8723ae/sw.c  |    2 -
>  drivers/net/wireless/realtek/rtlwifi/rtl8723be/sw.c  |    2 -
>  drivers/net/wireless/realtek/rtlwifi/rtl8821ae/sw.c  |    2 -
>  drivers/platform/chrome/chromeos_laptop.c            |   22 +++++++++----------
>  drivers/platform/x86/intel_scu_ipc.c                 |    6 ++---
>  drivers/platform/x86/intel_telemetry_debugfs.c       |    2 -
>  drivers/scsi/esas2r/esas2r_flash.c                   |    2 -
>  drivers/scsi/hptiop.c                                |    6 ++---
>  drivers/spi/spi-dw-pci.c                             |    4 +--
>  drivers/staging/rtl8192e/rtl8192e/rtl_core.c         |    2 -
>  drivers/usb/misc/ezusb.c                             |    2 -
>  drivers/video/fbdev/matrox/matroxfb_g450.c           |    2 -
>  lib/crc64_ecma.c                                     |    2 -
>  sound/pci/ctxfi/ctatc.c                              |    2 -
>  sound/pci/hda/patch_ca0132.c                         |   10 ++++----
>  sound/pci/riptide/riptide.c                          |    2 -
>  40 files changed, 110 insertions(+), 110 deletions(-)

Re: [PATCH 00/26] constify local structures

[Joe Perches]

On Sun, 2016-09-11 at 15:05 +0200, Julia Lawall wrote:
> Constify local structures.

Thanks Julia.

A few suggestions & questions:

Perhaps the script should go into scripts/coccinelle/
so that future cases could be caught by the robot
and commit message referenced by the patch instances.

Can you please compile the files modified using the
appropriate defconfig/allyesconfig and show the
movement from data to const by using
	$ size <object>.new/old
and include that in the changelogs (maybe next time)?

Is it possible for a rule to trace the instances where
an address of a struct or struct member is taken by
locally defined and declared function call where the
callee does not modify any dereferenced object?

ie:

struct foo {
	int bar;
	char *baz;
};

struct foo qux[] = {
	{ 1, "description 1" },
	{ 2, "dewcription 2" },
	[ n, "etc" ]...,
};

void message(struct foo *msg)
{
	printk("%d %s\n", msg->bar, msg->baz);
}

where some code uses

	message(qux[index]);

So could a coccinelle script change:

struct foo qux[] = { to const struct foo quz[] = {

and

void message(struct foo *msg) to void message(const struct foo *msg)

Re: [PATCH 00/26] constify local structures

[Julia Lawall]

On Sun, 11 Sep 2016, Joe Perches wrote:

> On Sun, 2016-09-11 at 15:05 +0200, Julia Lawall wrote:
> > Constify local structures.
>
> Thanks Julia.
>
> A few suggestions & questions:
>
> Perhaps the script should go into scripts/coccinelle/
> so that future cases could be caught by the robot
> and commit message referenced by the patch instances.

OK.

> Can you please compile the files modified using the
> appropriate defconfig/allyesconfig and show the

I currently send patches for this issue only for files that compile using
the x86 allyesconfig.

> movement from data to const by using
> 	$ size <object>.new/old
> and include that in the changelogs (maybe next time)?

OK, thanks for the suggestion.

> Is it possible for a rule to trace the instances where
> an address of a struct or struct member is taken by
> locally defined and declared function call where the
> callee does not modify any dereferenced object?
>
> ie:
>
> struct foo {
> 	int bar;
> 	char *baz;
> };
>
> struct foo qux[] = {
> 	{ 1, "description 1" },
> 	{ 2, "dewcription 2" },
> 	[ n, "etc" ]...,
> };
>
> void message(struct foo *msg)
> {
> 	printk("%d %s\n", msg->bar, msg->baz);
> }
>
> where some code uses
>
> 	message(qux[index]);
>
> So could a coccinelle script change:
>
> struct foo qux[] = { to const struct foo quz[] = {
>
> and
>
> void message(struct foo *msg) to void message(const struct foo *msg)

Yes, this could be possible too.

Thanks for the feedback.

julia

Re: [PATCH 00/26] constify local structures

[Julia Lawall]

On Sun, 11 Sep 2016, Jarkko Sakkinen wrote:

> On Sun, Sep 11, 2016 at 03:05:42PM +0200, Julia Lawall wrote:
> > Constify local structures.
> >
> > The semantic patch that makes this change is as follows:
> > (http://coccinelle.lip6.fr/)
>
> Just my two cents but:
>
> 1. You *can* use a static analysis too to find bugs or other issues.
> 2. However, you should manually do the commits and proper commit
>    messages to subsystems based on your findings. And I generally think
>    that if one contributes code one should also at least smoke test changes
>    somehow.
>
> I don't know if I'm alone with my opinion. I just think that one should
> also do the analysis part and not blindly create and submit patches.

All of the patches are compile tested.  And the individual patches are
submitted to the relevant maintainers.  The individual commit messages
give a more detailed explanation of the strategy used to decide that the
structure was constifiable.  It seemed redundant to put that in the cover
letter, which will not be committed anyway.

julia

>
> Anyway, I'll apply the TPM change at some point. As I said they were
> for better. Thanks.
>
> /Jarkko
>
> > // <smpl>
> > // The first rule ignores some cases that posed problems
> > @r disable optional_qualifier@
> > identifier s != {peri_clk_data,threshold_attr,tracer_flags,tracer};
> > identifier i != {s5k5baf_cis_rect,smtcfb_fix};
> > position p;
> > @@
> > static struct s i@p = { ... };
> >
> > @lstruct@
> > identifier r.s;
> > @@
> > struct s { ... };
> >
> > @used depends on lstruct@
> > identifier r.i;
> > @@
> > i
> >
> > @bad1@
> > expression e;
> > identifier r.i;
> > assignment operator a;
> > @@
> >  (<+...i...+>) a e
> >
> > @bad2@
> > identifier r.i;
> > @@
> >  &(<+...i...+>)
> >
> > @bad3@
> > identifier r.i;
> > declarer d;
> > @@
> >  d(...,<+...i...+>,...);
> >
> > @bad4@
> > identifier r.i;
> > type T;
> > T[] e;
> > identifier f;
> > position p;
> > @@
> >
> > f@p(...,
> > (
> >   (<+...i...+>)
> > &
> >   e
> > )
> > ,...)
> >
> > @bad4a@
> > identifier r.i;
> > type T;
> > T *e;
> > identifier f;
> > position p;
> > @@
> >
> > f@p(...,
> > (
> >   (<+...i...+>)
> > &
> >   e
> > )
> > ,...)
> >
> > @ok5@
> > expression *e;
> > identifier r.i;
> > position p;
> > @@
> > e =@p i
> >
> > @bad5@
> > expression *e;
> > identifier r.i;
> > position p != ok5.p;
> > @@
> > e =@p (<+...i...+>)
> >
> > @rr depends on used && !bad1 && !bad2 && !bad3 && !bad4 && !bad4a && !bad5@
> > identifier s,r.i;
> > position r.p;
> > @@
> >
> > static
> > +const
> >  struct s i@p = { ... };
> >
> > @depends on used && !bad1 && !bad2 && !bad3 && !bad4 && !bad4a && !bad5
> >  disable optional_qualifier@
> > identifier rr.s,r.i;
> > @@
> >
> > static
> > +const
> >  struct s i;
> > // </smpl>
> >
> > ---
> >
> >  drivers/acpi/acpi_apd.c                              |    8 +++---
> >  drivers/char/tpm/tpm-interface.c                     |   10 ++++----
> >  drivers/char/tpm/tpm-sysfs.c                         |    2 -
> >  drivers/cpufreq/intel_pstate.c                       |    8 +++---
> >  drivers/infiniband/hw/i40iw/i40iw_uk.c               |    6 ++---
> >  drivers/media/i2c/tvp514x.c                          |    2 -
> >  drivers/media/pci/ddbridge/ddbridge-core.c           |   18 +++++++--------
> >  drivers/media/pci/ngene/ngene-cards.c                |   14 ++++++------
> >  drivers/media/pci/smipcie/smipcie-main.c             |    8 +++---
> >  drivers/misc/sgi-xp/xpc_uv.c                         |    2 -
> >  drivers/net/arcnet/com20020-pci.c                    |   10 ++++----
> >  drivers/net/can/c_can/c_can_pci.c                    |    4 +--
> >  drivers/net/can/sja1000/plx_pci.c                    |   20 ++++++++---------
> >  drivers/net/ethernet/mellanox/mlx4/main.c            |    4 +--
> >  drivers/net/ethernet/oki-semi/pch_gbe/pch_gbe_main.c |    2 -
> >  drivers/net/ethernet/renesas/sh_eth.c                |   14 ++++++------
> >  drivers/net/ethernet/stmicro/stmmac/stmmac_pci.c     |    2 -
> >  drivers/net/wireless/ath/dfs_pattern_detector.c      |    2 -
> >  drivers/net/wireless/intel/iwlegacy/3945.c           |    4 +--
> >  drivers/net/wireless/realtek/rtlwifi/rtl8188ee/sw.c  |    2 -
> >  drivers/net/wireless/realtek/rtlwifi/rtl8192ce/sw.c  |    2 -
> >  drivers/net/wireless/realtek/rtlwifi/rtl8192de/sw.c  |    2 -
> >  drivers/net/wireless/realtek/rtlwifi/rtl8192ee/sw.c  |    2 -
> >  drivers/net/wireless/realtek/rtlwifi/rtl8192se/sw.c  |    2 -
> >  drivers/net/wireless/realtek/rtlwifi/rtl8723ae/sw.c  |    2 -
> >  drivers/net/wireless/realtek/rtlwifi/rtl8723be/sw.c  |    2 -
> >  drivers/net/wireless/realtek/rtlwifi/rtl8821ae/sw.c  |    2 -
> >  drivers/platform/chrome/chromeos_laptop.c            |   22 +++++++++----------
> >  drivers/platform/x86/intel_scu_ipc.c                 |    6 ++---
> >  drivers/platform/x86/intel_telemetry_debugfs.c       |    2 -
> >  drivers/scsi/esas2r/esas2r_flash.c                   |    2 -
> >  drivers/scsi/hptiop.c                                |    6 ++---
> >  drivers/spi/spi-dw-pci.c                             |    4 +--
> >  drivers/staging/rtl8192e/rtl8192e/rtl_core.c         |    2 -
> >  drivers/usb/misc/ezusb.c                             |    2 -
> >  drivers/video/fbdev/matrox/matroxfb_g450.c           |    2 -
> >  lib/crc64_ecma.c                                     |    2 -
> >  sound/pci/ctxfi/ctatc.c                              |    2 -
> >  sound/pci/hda/patch_ca0132.c                         |   10 ++++----
> >  sound/pci/riptide/riptide.c                          |    2 -
> >  40 files changed, 110 insertions(+), 110 deletions(-)
>

Re: [PATCH 00/26] constify local structures

[Jarkko Sakkinen]

On Mon, Sep 12, 2016 at 10:54:07AM +0200, Julia Lawall wrote:
> 
> 
> On Sun, 11 Sep 2016, Jarkko Sakkinen wrote:
> 
> > On Sun, Sep 11, 2016 at 03:05:42PM +0200, Julia Lawall wrote:
> > > Constify local structures.
> > >
> > > The semantic patch that makes this change is as follows:
> > > (http://coccinelle.lip6.fr/)
> >
> > Just my two cents but:
> >
> > 1. You *can* use a static analysis too to find bugs or other issues.
> > 2. However, you should manually do the commits and proper commit
> >    messages to subsystems based on your findings. And I generally think
> >    that if one contributes code one should also at least smoke test changes
> >    somehow.
> >
> > I don't know if I'm alone with my opinion. I just think that one should
> > also do the analysis part and not blindly create and submit patches.
> 
> All of the patches are compile tested.  And the individual patches are

Compile-testing is not testing. If you are not able to test a commit,
you should explain why.

> submitted to the relevant maintainers.  The individual commit messages
> give a more detailed explanation of the strategy used to decide that the
> structure was constifiable.  It seemed redundant to put that in the cover
> letter, which will not be committed anyway.

I don't mean to be harsh but I do not care about your thought process
*that much* when I review a commit (sometimes it might make sense to
explain that but it depends on the context).

I mostly only care why a particular change makes sense for this
particular subsystem. The report given by a static analysis tool can
be a starting point for making a commit but it's not sufficient.
Based on the report you should look subsystems as individuals.

> julia

/Jarkko

Re: [PATCH 00/26] constify local structures

[Julia Lawall]

On Mon, 12 Sep 2016, Jarkko Sakkinen wrote:

> On Mon, Sep 12, 2016 at 10:54:07AM +0200, Julia Lawall wrote:
> >
> >
> > On Sun, 11 Sep 2016, Jarkko Sakkinen wrote:
> >
> > > On Sun, Sep 11, 2016 at 03:05:42PM +0200, Julia Lawall wrote:
> > > > Constify local structures.
> > > >
> > > > The semantic patch that makes this change is as follows:
> > > > (http://coccinelle.lip6.fr/)
> > >
> > > Just my two cents but:
> > >
> > > 1. You *can* use a static analysis too to find bugs or other issues.
> > > 2. However, you should manually do the commits and proper commit
> > >    messages to subsystems based on your findings. And I generally think
> > >    that if one contributes code one should also at least smoke test changes
> > >    somehow.
> > >
> > > I don't know if I'm alone with my opinion. I just think that one should
> > > also do the analysis part and not blindly create and submit patches.
> >
> > All of the patches are compile tested.  And the individual patches are
>
> Compile-testing is not testing. If you are not able to test a commit,
> you should explain why.
>
> > submitted to the relevant maintainers.  The individual commit messages
> > give a more detailed explanation of the strategy used to decide that the
> > structure was constifiable.  It seemed redundant to put that in the cover
> > letter, which will not be committed anyway.
>
> I don't mean to be harsh but I do not care about your thought process
> *that much* when I review a commit (sometimes it might make sense to
> explain that but it depends on the context).
>
> I mostly only care why a particular change makes sense for this
> particular subsystem. The report given by a static analysis tool can
> be a starting point for making a commit but it's not sufficient.
> Based on the report you should look subsystems as individuals.

OK, thanks for the feedback.

julia

Re: [PATCH 00/26] constify local structures

[Felipe Balbi]

[-- Attachment #1: Type: text/plain, Size: 1837 bytes --]


Hi,

Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com> writes:
> On Mon, Sep 12, 2016 at 10:54:07AM +0200, Julia Lawall wrote:
>> 
>> 
>> On Sun, 11 Sep 2016, Jarkko Sakkinen wrote:
>> 
>> > On Sun, Sep 11, 2016 at 03:05:42PM +0200, Julia Lawall wrote:
>> > > Constify local structures.
>> > >
>> > > The semantic patch that makes this change is as follows:
>> > > (http://coccinelle.lip6.fr/)
>> >
>> > Just my two cents but:
>> >
>> > 1. You *can* use a static analysis too to find bugs or other issues.
>> > 2. However, you should manually do the commits and proper commit
>> >    messages to subsystems based on your findings. And I generally think
>> >    that if one contributes code one should also at least smoke test changes
>> >    somehow.
>> >
>> > I don't know if I'm alone with my opinion. I just think that one should
>> > also do the analysis part and not blindly create and submit patches.
>> 
>> All of the patches are compile tested.  And the individual patches are
>
> Compile-testing is not testing. If you are not able to test a commit,
> you should explain why.

Dude, Julia has been doing semantic patching for years already and
nobody has raised any concerns so far. There's already an expectation
that Coccinelle *works* and Julia's sematic patches are sound.

Besides, adding 'const' is something that causes virtually no functional
changes to the point that build-testing is really all you need. Any
problems caused by adding 'const' to a definition will be seen by build
errors or warnings.

Really, just stop with the pointless discussion and go read a bit about
Coccinelle and what semantic patches are giving you. The work done by
Julia and her peers are INRIA have measurable benefits.

You're really making a thunderstorm in a glass of water.

-- 
balbi

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 800 bytes --]

Re: [PATCH 00/26] constify local structures

[Julia Lawall]

On Mon, 12 Sep 2016, Felipe Balbi wrote:

>
> Hi,
>
> Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com> writes:
> > On Mon, Sep 12, 2016 at 10:54:07AM +0200, Julia Lawall wrote:
> >>
> >>
> >> On Sun, 11 Sep 2016, Jarkko Sakkinen wrote:
> >>
> >> > On Sun, Sep 11, 2016 at 03:05:42PM +0200, Julia Lawall wrote:
> >> > > Constify local structures.
> >> > >
> >> > > The semantic patch that makes this change is as follows:
> >> > > (http://coccinelle.lip6.fr/)
> >> >
> >> > Just my two cents but:
> >> >
> >> > 1. You *can* use a static analysis too to find bugs or other issues.
> >> > 2. However, you should manually do the commits and proper commit
> >> >    messages to subsystems based on your findings. And I generally think
> >> >    that if one contributes code one should also at least smoke test changes
> >> >    somehow.
> >> >
> >> > I don't know if I'm alone with my opinion. I just think that one should
> >> > also do the analysis part and not blindly create and submit patches.
> >>
> >> All of the patches are compile tested.  And the individual patches are
> >
> > Compile-testing is not testing. If you are not able to test a commit,
> > you should explain why.
>
> Dude, Julia has been doing semantic patching for years already and
> nobody has raised any concerns so far. There's already an expectation
> that Coccinelle *works* and Julia's sematic patches are sound.
>
> Besides, adding 'const' is something that causes virtually no functional
> changes to the point that build-testing is really all you need. Any
> problems caused by adding 'const' to a definition will be seen by build
> errors or warnings.
>
> Really, just stop with the pointless discussion and go read a bit about
> Coccinelle and what semantic patches are giving you. The work done by
> Julia and her peers are INRIA have measurable benefits.
>
> You're really making a thunderstorm in a glass of water.

Thanks for the defense, but since a lot of these patches torned out to be
wrong, due to an incorrect parse by Coccinelle, combined with an
unpleasantly lax compiler, Jarkko does have a point that I should have
looked at the patches more carefully.  In any case, I have written to the
maintainers relevant to the patches that turned out to be incorrect.

julia

Re: [PATCH 00/26] constify local structures

[Geert Uytterhoeven]

On Mon, Sep 12, 2016 at 3:43 PM, Felipe Balbi
<felipe.balbi@linux.intel.com> wrote:
> Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com> writes:
>> On Mon, Sep 12, 2016 at 10:54:07AM +0200, Julia Lawall wrote:
>>> On Sun, 11 Sep 2016, Jarkko Sakkinen wrote:
>>> > On Sun, Sep 11, 2016 at 03:05:42PM +0200, Julia Lawall wrote:
>>> > > Constify local structures.
>>> > >
>>> > > The semantic patch that makes this change is as follows:
>>> > > (http://coccinelle.lip6.fr/)
>>> >
>>> > Just my two cents but:
>>> >
>>> > 1. You *can* use a static analysis too to find bugs or other issues.
>>> > 2. However, you should manually do the commits and proper commit
>>> >    messages to subsystems based on your findings. And I generally think
>>> >    that if one contributes code one should also at least smoke test changes
>>> >    somehow.
>>> >
>>> > I don't know if I'm alone with my opinion. I just think that one should
>>> > also do the analysis part and not blindly create and submit patches.
>>>
>>> All of the patches are compile tested.  And the individual patches are
>>
>> Compile-testing is not testing. If you are not able to test a commit,
>> you should explain why.
>
> Dude, Julia has been doing semantic patching for years already and
> nobody has raised any concerns so far. There's already an expectation
> that Coccinelle *works* and Julia's sematic patches are sound.

+1

> Besides, adding 'const' is something that causes virtually no functional
> changes to the point that build-testing is really all you need. Any
> problems caused by adding 'const' to a definition will be seen by build
> errors or warnings.

Unfortunately in this particular case they could lead to failures that can only
be detected at runtime, when failing o write to a read-only piece of memory,
due to the casting away of the constness of the pointers later.
Fortunately this was detected during code review (doh...).

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH 00/26] constify local structures

[Jarkko Sakkinen]

On Mon, Sep 12, 2016 at 03:52:08PM +0200, Julia Lawall wrote:
> 
> 
> On Mon, 12 Sep 2016, Felipe Balbi wrote:
> 
> >
> > Hi,
> >
> > Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com> writes:
> > > On Mon, Sep 12, 2016 at 10:54:07AM +0200, Julia Lawall wrote:
> > >>
> > >>
> > >> On Sun, 11 Sep 2016, Jarkko Sakkinen wrote:
> > >>
> > >> > On Sun, Sep 11, 2016 at 03:05:42PM +0200, Julia Lawall wrote:
> > >> > > Constify local structures.
> > >> > >
> > >> > > The semantic patch that makes this change is as follows:
> > >> > > (http://coccinelle.lip6.fr/)
> > >> >
> > >> > Just my two cents but:
> > >> >
> > >> > 1. You *can* use a static analysis too to find bugs or other issues.
> > >> > 2. However, you should manually do the commits and proper commit
> > >> >    messages to subsystems based on your findings. And I generally think
> > >> >    that if one contributes code one should also at least smoke test changes
> > >> >    somehow.
> > >> >
> > >> > I don't know if I'm alone with my opinion. I just think that one should
> > >> > also do the analysis part and not blindly create and submit patches.
> > >>
> > >> All of the patches are compile tested.  And the individual patches are
> > >
> > > Compile-testing is not testing. If you are not able to test a commit,
> > > you should explain why.
> >
> > Dude, Julia has been doing semantic patching for years already and
> > nobody has raised any concerns so far. There's already an expectation
> > that Coccinelle *works* and Julia's sematic patches are sound.
> >
> > Besides, adding 'const' is something that causes virtually no functional
> > changes to the point that build-testing is really all you need. Any
> > problems caused by adding 'const' to a definition will be seen by build
> > errors or warnings.
> >
> > Really, just stop with the pointless discussion and go read a bit about
> > Coccinelle and what semantic patches are giving you. The work done by
> > Julia and her peers are INRIA have measurable benefits.
> >
> > You're really making a thunderstorm in a glass of water.
> 
> Thanks for the defense, but since a lot of these patches torned out to be
> wrong, due to an incorrect parse by Coccinelle, combined with an
> unpleasantly lax compiler, Jarkko does have a point that I should have
> looked at the patches more carefully.  In any case, I have written to the
> maintainers relevant to the patches that turned out to be incorrect.

Exactly. I'm not excepting that every commit would require extensive
analysis but it would be good to quickly at least skim through commits
and see if they make sense (or ask if unsure) :) 

And I'm fine with compile testing if it is mentioned in the commit msg.

> julia

/Jarkko

Re: [PATCH 00/26] constify local structures

[Jarkko Sakkinen]

On Mon, Sep 12, 2016 at 04:43:58PM +0300, Felipe Balbi wrote:
> 
> Hi,
> 
> Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com> writes:
> > On Mon, Sep 12, 2016 at 10:54:07AM +0200, Julia Lawall wrote:
> >> 
> >> 
> >> On Sun, 11 Sep 2016, Jarkko Sakkinen wrote:
> >> 
> >> > On Sun, Sep 11, 2016 at 03:05:42PM +0200, Julia Lawall wrote:
> >> > > Constify local structures.
> >> > >
> >> > > The semantic patch that makes this change is as follows:
> >> > > (http://coccinelle.lip6.fr/)
> >> >
> >> > Just my two cents but:
> >> >
> >> > 1. You *can* use a static analysis too to find bugs or other issues.
> >> > 2. However, you should manually do the commits and proper commit
> >> >    messages to subsystems based on your findings. And I generally think
> >> >    that if one contributes code one should also at least smoke test changes
> >> >    somehow.
> >> >
> >> > I don't know if I'm alone with my opinion. I just think that one should
> >> > also do the analysis part and not blindly create and submit patches.
> >> 
> >> All of the patches are compile tested.  And the individual patches are
> >
> > Compile-testing is not testing. If you are not able to test a commit,
> > you should explain why.
> 
> Dude, Julia has been doing semantic patching for years already and
> nobody has raised any concerns so far. There's already an expectation
> that Coccinelle *works* and Julia's sematic patches are sound.
> 
> Besides, adding 'const' is something that causes virtually no functional
> changes to the point that build-testing is really all you need. Any
> problems caused by adding 'const' to a definition will be seen by build
> errors or warnings.
> 
> Really, just stop with the pointless discussion and go read a bit about
> Coccinelle and what semantic patches are giving you. The work done by
> Julia and her peers are INRIA have measurable benefits.
> 
> You're really making a thunderstorm in a glass of water.

Hmm... I've been using coccinelle in cyclic basis for some time now.
My comment was oversized but I didn't mean it to be impolite or attack
of any kind for that matter.

> -- 
> balbi

/Jarkko

Re: [PATCH 00/26] constify local structures

[Julia Lawall]

On Mon, 12 Sep 2016, Jarkko Sakkinen wrote:

> On Mon, Sep 12, 2016 at 04:43:58PM +0300, Felipe Balbi wrote:
> >
> > Hi,
> >
> > Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com> writes:
> > > On Mon, Sep 12, 2016 at 10:54:07AM +0200, Julia Lawall wrote:
> > >>
> > >>
> > >> On Sun, 11 Sep 2016, Jarkko Sakkinen wrote:
> > >>
> > >> > On Sun, Sep 11, 2016 at 03:05:42PM +0200, Julia Lawall wrote:
> > >> > > Constify local structures.
> > >> > >
> > >> > > The semantic patch that makes this change is as follows:
> > >> > > (http://coccinelle.lip6.fr/)
> > >> >
> > >> > Just my two cents but:
> > >> >
> > >> > 1. You *can* use a static analysis too to find bugs or other issues.
> > >> > 2. However, you should manually do the commits and proper commit
> > >> >    messages to subsystems based on your findings. And I generally think
> > >> >    that if one contributes code one should also at least smoke test changes
> > >> >    somehow.
> > >> >
> > >> > I don't know if I'm alone with my opinion. I just think that one should
> > >> > also do the analysis part and not blindly create and submit patches.
> > >>
> > >> All of the patches are compile tested.  And the individual patches are
> > >
> > > Compile-testing is not testing. If you are not able to test a commit,
> > > you should explain why.
> >
> > Dude, Julia has been doing semantic patching for years already and
> > nobody has raised any concerns so far. There's already an expectation
> > that Coccinelle *works* and Julia's sematic patches are sound.
> >
> > Besides, adding 'const' is something that causes virtually no functional
> > changes to the point that build-testing is really all you need. Any
> > problems caused by adding 'const' to a definition will be seen by build
> > errors or warnings.
> >
> > Really, just stop with the pointless discussion and go read a bit about
> > Coccinelle and what semantic patches are giving you. The work done by
> > Julia and her peers are INRIA have measurable benefits.
> >
> > You're really making a thunderstorm in a glass of water.
>
> Hmm... I've been using coccinelle in cyclic basis for some time now.
> My comment was oversized but I didn't mean it to be impolite or attack
> of any kind for that matter.

No problem :)  Thanks for the feedback.

julia