| Message ID | 1518622745-12162-2-git-send-email-festevam@gmail.com (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | 91cd00083d734258bf293dcf76793e2348be391a |
| Headers | show |
On Wed, Feb 14, 2018 at 01:39:04PM -0200, Fabio Estevam wrote: > From: Fabio Estevam <fabio.estevam@nxp.com> > > { 0 } only clears the first member of the structure. > > The first member of the snd_soc_dapm_update struct is a pointer, > and writing 0 to a pointer results in a sparse warning. > > Use the empty struct initializer that clears all the struct members > and fixes the sparse warning. > > Cc: Charles Keepax <ckeepax@opensource.cirrus.com> > Signed-off-by: Fabio Estevam <fabio.estevam@nxp.com> > --- Acked-by: Charles Keepax <ckeepax@opensource.cirrus.com> Thanks, Charles
On 14/02/18 15:39, Fabio Estevam wrote: > From: Fabio Estevam <fabio.estevam@nxp.com> > > { 0 } only clears the first member of the structure. > Not according to the C99 and C11 standards. C11 6.7.9.21 (C99 has the same statement): If there are fewer initializers in a brace-enclosed list than there are elements or members of an aggregate, or fewer characters in a string literal used to initialize an array of known size than there are elements in the array, the remainder of the aggregate shall be initialized implicitly the same as objects that have static storage duration. C11 6.7.9.10 defines that "objects that have static storage duration" shall be initialized to zero. So according to both C11 and C99 standards {0} should (and must) initialize the entire structure to zero. > The first member of the snd_soc_dapm_update struct is a pointer, > and writing 0 to a pointer results in a sparse warning. > This is the actual problem you are trying to fix? The comment about { 0 } only clearing the first member is probably bogus, your actual problem is that it should have been { NULL } ? So the fix works (because of 6.7.9.21 quoted above) but the commit message is incorrect/misleading about what the problem is and why this is a fix for it. > Use the empty struct initializer that clears all the struct members > and fixes the sparse warning. > > Cc: Charles Keepax <ckeepax@opensource.cirrus.com> > Signed-off-by: Fabio Estevam <fabio.estevam@nxp.com> > --- > sound/soc/codecs/wm9713.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/sound/soc/codecs/wm9713.c b/sound/soc/codecs/wm9713.c > index 3d6cf00..643863b 100644 > --- a/sound/soc/codecs/wm9713.c > +++ b/sound/soc/codecs/wm9713.c > @@ -235,7 +235,7 @@ static int wm9713_hp_mixer_put(struct snd_kcontrol *kcontrol, > struct soc_mixer_control *mc = > (struct soc_mixer_control *)kcontrol->private_value; > unsigned int mixer, mask, shift, old; > - struct snd_soc_dapm_update update = { 0 }; > + struct snd_soc_dapm_update update = {}; > bool change; > > mixer = mc->shift >> 8; >
On Wed, Feb 14, 2018 at 2:41 PM, Richard Fitzgerald <rf@opensource.cirrus.com> wrote: > This is the actual problem you are trying to fix? The comment about > { 0 } only clearing the first member is probably bogus, your actual > problem is that it should have been { NULL } ? Changing to { NULL } fixes the sparse warning, but that would be a fragile fix because it relies on the order of the struct members. If someday the struct changes in a way so that the first member is no longer a pointer, then we will have issues again. Using {} is more error prone.
On Wed, Feb 14, 2018 at 2:49 PM, Fabio Estevam <festevam@gmail.com> wrote:
> Using {} is more error prone.
I mean, less error prone :-)
On 14/02/18 16:49, Fabio Estevam wrote: > On Wed, Feb 14, 2018 at 2:41 PM, Richard Fitzgerald > <rf@opensource.cirrus.com> wrote: > >> This is the actual problem you are trying to fix? The comment about >> { 0 } only clearing the first member is probably bogus, your actual >> problem is that it should have been { NULL } ? > > Changing to { NULL } fixes the sparse warning, but that would be a > fragile fix because it relies on the order of the struct members. > > If someday the struct changes in a way so that the first member is no > longer a pointer, then we will have issues again. > > Using {} is more error prone. > I agree but your commit message didn't say that. It's irrelevant now Mark has merged the patch, but for anyone looking at the commit message later, the structure of your commit message implies that the problem is that {0} doesn't work correctly (probably untrue and certainly not the actual problem.) and your actual fix relies on precisely the behaviour that the first line of your commit message implies is broken. I just like commit messages to be accurate about what the problem was and why. It's often said that an incorrect comment is worse than not having the comment at all, and the same could be said for commit messages,
On Wed, Feb 14, 2018 at 2:56 PM, Richard Fitzgerald <rf@opensource.cirrus.com> wrote: > I agree but your commit message didn't say that. It's irrelevant now > Mark has merged the patch, but for anyone looking at the commit message > later, the structure of your commit message implies that the problem > is that {0} doesn't work correctly (probably untrue and certainly not Yes, and that is the message I want to give: {0} does not work correctly if the first member of the struct is a pointer, as we should not explicitly write 0 to pointers.
On 14/02/18 17:01, Fabio Estevam wrote: > On Wed, Feb 14, 2018 at 2:56 PM, Richard Fitzgerald > <rf@opensource.cirrus.com> wrote: > >> I agree but your commit message didn't say that. It's irrelevant now >> Mark has merged the patch, but for anyone looking at the commit message >> later, the structure of your commit message implies that the problem >> is that {0} doesn't work correctly (probably untrue and certainly not > > Yes, and that is the message I want to give: > > {0} does not work correctly if the first member of the struct is a > pointer, as we should not explicitly write 0 to pointers. > I gather that, so I'd have liked to your commit message to have said that. But what you actually said was "{ 0 } only clears the first member of the structure." which is a very different message.
On Wed, Feb 14, 2018 at 3:15 PM, Richard Fitzgerald <rf@opensource.cirrus.com> wrote: > I gather that, so I'd have liked to your commit message to have said that. > But what you actually said was > > "{ 0 } only clears the first member of the structure." > > which is a very different message. Got it. Maybe I should have written like this instead: "{ 0 } only clears explicitly the first member of the structure." Thanks for the feedback.
diff --git a/sound/soc/codecs/wm9713.c b/sound/soc/codecs/wm9713.c index 3d6cf00..643863b 100644 --- a/sound/soc/codecs/wm9713.c +++ b/sound/soc/codecs/wm9713.c @@ -235,7 +235,7 @@ static int wm9713_hp_mixer_put(struct snd_kcontrol *kcontrol, struct soc_mixer_control *mc = (struct soc_mixer_control *)kcontrol->private_value; unsigned int mixer, mask, shift, old; - struct snd_soc_dapm_update update = { 0 }; + struct snd_soc_dapm_update update = {}; bool change; mixer = mc->shift >> 8;