[alsa-devel] [PATCH 00/26] constify local structures
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(-)
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.
sound/pci/ctxfi/ctatc.c | 2 +- sound/pci/hda/patch_ca0132.c | 10 +++++----- sound/pci/riptide/riptide.c | 2 +- 3 files changed, 7 insertions(+), 7 deletions(-)
diff --git a/sound/pci/ctxfi/ctatc.c b/sound/pci/ctxfi/ctatc.c index 977a598..908658a 100644 --- a/sound/pci/ctxfi/ctatc.c +++ b/sound/pci/ctxfi/ctatc.c @@ -1623,7 +1623,7 @@ static int atc_resume(struct ct_atc *atc) } #endif
-static struct ct_atc atc_preset = { +static const struct ct_atc atc_preset = { .map_audio_buffer = ct_map_audio_buffer, .unmap_audio_buffer = ct_unmap_audio_buffer, .pcm_playback_prepare = atc_pcm_playback_prepare, diff --git a/sound/pci/hda/patch_ca0132.c b/sound/pci/hda/patch_ca0132.c index 9ceb2bc..ad06866 100644 --- a/sound/pci/hda/patch_ca0132.c +++ b/sound/pci/hda/patch_ca0132.c @@ -4018,7 +4018,7 @@ static int ca0132_build_controls(struct hda_codec *codec) /* * PCM */ -static struct hda_pcm_stream ca0132_pcm_analog_playback = { +static const struct hda_pcm_stream ca0132_pcm_analog_playback = { .substreams = 1, .channels_min = 2, .channels_max = 6, @@ -4029,7 +4029,7 @@ static struct hda_pcm_stream ca0132_pcm_analog_playback = { }, };
-static struct hda_pcm_stream ca0132_pcm_analog_capture = { +static const struct hda_pcm_stream ca0132_pcm_analog_capture = { .substreams = 1, .channels_min = 2, .channels_max = 2, @@ -4040,7 +4040,7 @@ static struct hda_pcm_stream ca0132_pcm_analog_capture = { }, };
-static struct hda_pcm_stream ca0132_pcm_digital_playback = { +static const struct hda_pcm_stream ca0132_pcm_digital_playback = { .substreams = 1, .channels_min = 2, .channels_max = 2, @@ -4052,7 +4052,7 @@ static struct hda_pcm_stream ca0132_pcm_digital_playback = { }, };
-static struct hda_pcm_stream ca0132_pcm_digital_capture = { +static const struct hda_pcm_stream ca0132_pcm_digital_capture = { .substreams = 1, .channels_min = 2, .channels_max = 2, @@ -4614,7 +4614,7 @@ static void ca0132_free(struct hda_codec *codec) kfree(codec->spec); }
-static struct hda_codec_ops ca0132_patch_ops = { +static const struct hda_codec_ops ca0132_patch_ops = { .build_controls = ca0132_build_controls, .build_pcms = ca0132_build_pcms, .init = ca0132_init, diff --git a/sound/pci/riptide/riptide.c b/sound/pci/riptide/riptide.c index ae41fcb..ada5f01 100644 --- a/sound/pci/riptide/riptide.c +++ b/sound/pci/riptide/riptide.c @@ -644,7 +644,7 @@ static struct lbuspath lbus_play_paths[] = { .mono = lbus_play_mono3, }, }; -static struct lbuspath lbus_rec_path = { +static const struct lbuspath lbus_rec_path = { .noconv = lbus_rec_noconv1, .stereo = lbus_rec_stereo1, .mono = lbus_rec_mono1,
On Sun, 11 Sep 2016 15:05:43 +0200, Julia Lawall wrote:
For structure types defined in the same file or local header files, find top-level static structure declarations that have the following properties:
- Never reassigned.
- Address never taken
- Not passed to a top-level macro call
- 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
Thanks, applied now.
Takashi
The semantic patch seems too long for a commit log, but is in the cover letter.
sound/pci/ctxfi/ctatc.c | 2 +- sound/pci/hda/patch_ca0132.c | 10 +++++----- sound/pci/riptide/riptide.c | 2 +- 3 files changed, 7 insertions(+), 7 deletions(-)
diff --git a/sound/pci/ctxfi/ctatc.c b/sound/pci/ctxfi/ctatc.c index 977a598..908658a 100644 --- a/sound/pci/ctxfi/ctatc.c +++ b/sound/pci/ctxfi/ctatc.c @@ -1623,7 +1623,7 @@ static int atc_resume(struct ct_atc *atc) } #endif
-static struct ct_atc atc_preset = { +static const struct ct_atc atc_preset = { .map_audio_buffer = ct_map_audio_buffer, .unmap_audio_buffer = ct_unmap_audio_buffer, .pcm_playback_prepare = atc_pcm_playback_prepare, diff --git a/sound/pci/hda/patch_ca0132.c b/sound/pci/hda/patch_ca0132.c index 9ceb2bc..ad06866 100644 --- a/sound/pci/hda/patch_ca0132.c +++ b/sound/pci/hda/patch_ca0132.c @@ -4018,7 +4018,7 @@ static int ca0132_build_controls(struct hda_codec *codec) /*
- PCM
*/ -static struct hda_pcm_stream ca0132_pcm_analog_playback = { +static const struct hda_pcm_stream ca0132_pcm_analog_playback = { .substreams = 1, .channels_min = 2, .channels_max = 6, @@ -4029,7 +4029,7 @@ static struct hda_pcm_stream ca0132_pcm_analog_playback = { }, };
-static struct hda_pcm_stream ca0132_pcm_analog_capture = { +static const struct hda_pcm_stream ca0132_pcm_analog_capture = { .substreams = 1, .channels_min = 2, .channels_max = 2, @@ -4040,7 +4040,7 @@ static struct hda_pcm_stream ca0132_pcm_analog_capture = { }, };
-static struct hda_pcm_stream ca0132_pcm_digital_playback = { +static const struct hda_pcm_stream ca0132_pcm_digital_playback = { .substreams = 1, .channels_min = 2, .channels_max = 2, @@ -4052,7 +4052,7 @@ static struct hda_pcm_stream ca0132_pcm_digital_playback = { }, };
-static struct hda_pcm_stream ca0132_pcm_digital_capture = { +static const struct hda_pcm_stream ca0132_pcm_digital_capture = { .substreams = 1, .channels_min = 2, .channels_max = 2, @@ -4614,7 +4614,7 @@ static void ca0132_free(struct hda_codec *codec) kfree(codec->spec); }
-static struct hda_codec_ops ca0132_patch_ops = { +static const struct hda_codec_ops ca0132_patch_ops = { .build_controls = ca0132_build_controls, .build_pcms = ca0132_build_pcms, .init = ca0132_init, diff --git a/sound/pci/riptide/riptide.c b/sound/pci/riptide/riptide.c index ae41fcb..ada5f01 100644 --- a/sound/pci/riptide/riptide.c +++ b/sound/pci/riptide/riptide.c @@ -644,7 +644,7 @@ static struct lbuspath lbus_play_paths[] = { .mono = lbus_play_mono3, }, }; -static struct lbuspath lbus_rec_path = { +static const struct lbuspath lbus_rec_path = { .noconv = lbus_rec_noconv1, .stereo = lbus_rec_stereo1, .mono = lbus_rec_mono1,
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(-)
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:
- You *can* use a static analysis too to find bugs or other issues.
- 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(-)
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:
- You *can* use a static analysis too to find bugs or other issues.
- 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
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:
- You *can* use a static analysis too to find bugs or other issues.
- 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
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:
- You *can* use a static analysis too to find bugs or other issues.
- 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.
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:
- You *can* use a static analysis too to find bugs or other issues.
- 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
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:
- You *can* use a static analysis too to find bugs or other issues.
- 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
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:
- You *can* use a static analysis too to find bugs or other issues.
- 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
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:
- You *can* use a static analysis too to find bugs or other issues.
- 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
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:
- You *can* use a static analysis too to find bugs or other issues.
- 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
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)
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
participants (6)
-
Felipe Balbi -
Geert Uytterhoeven -
Jarkko Sakkinen -
Joe Perches -
Julia Lawall -
Takashi Iwai