On Mon, 22 Feb 2016 12:07:54 +0100,
Takashi Sakamoto wrote:
>
> On Feb 22 2016 18:51, Takashi Iwai wrote:
> > On Mon, 22 Feb 2016 01:34:30 +0100,
> > Takashi Sakamoto wrote:
> >>
> >> Hi,
> >>
> >> On Feb 22 2016 02:46, Takashi Iwai wrote:
> >>> On Sun, 21 Feb 2016 17:54:09 +0100,
> >>> Takashi Sakamoto wrote:
> >>>>
> >>>> The assert(3) aborts running process when expression arguments is
> >>>> false (zero). Although, the snd_ctl_elem_set_bytes() has return
> >>>> statement after assert(3). It has meaningless.
> >>>
> >>> Admittedly this isn't an optimal code, but it has its purpose.
> >>> The intention of the original code was to perform the check always but
> >>> triggers the assert signal unless NDEBUG is set (remember that
> >>> assert() is nop when NDEBUG is defined).
> >>
> >> Although I realized the effect of NDEBUG for calls of asset(3), I didn't
> >> realize the intension to the code, because the other parts of
> >> 'src/control/control.c' are written without such intention. We can see
> >> many assert(3)s are just used without conditional statements.
> >>
> >> Hm. I think it better to keep consistency of whole codes rather than
> >> such partial intensions. On the other hand, this patch changes library
> >> behaviour. It's not generally allowed, but here, I think it better
> >> because of the consistency and the actual effect.
> >
> > Yeah, that's mostly OK to make it a simple assert.
> >
> > Or, if I may judge only from my taste, even removing the assert would
> > be nicer. Triggering assert() is one of the worst behavior as a base
> > system library.
>
> I completely agree with the idea ;)
>
> Although, it's out of my plan in this developing cycle. In this time, I
> hope just to apply this patch and go to my next work for control element
> set APIs. I should post revised version?
Well, removing assert() is somewhat radical and needs more consensus.
So, although I'd love it, let's make this patch in. But, for that,
I'd need a slightly corrected change log text. Care to resubmit after
rephrasing?
thanks,
Takashi
>
> Regards
>
> >>>> This commit corrects the statements.
> >>>>
> >>>> Fixes: 7893ea238d5a('Added mode argument to open functions where it was missing. First part of CTL documentation')
> >>>> Signed-off-by: Takashi Sakamoto <o-takashi@xxxxxxxxxxxxx>
> >>>> ---
> >>>> src/control/control.c | 5 +----
> >>>> 1 file changed, 1 insertion(+), 4 deletions(-)
> >>>>
> >>>> diff --git a/src/control/control.c b/src/control/control.c
> >>>> index 328920d..70d168d 100644
> >>>> --- a/src/control/control.c
> >>>> +++ b/src/control/control.c
> >>>> @@ -2627,10 +2627,7 @@ void snd_ctl_elem_value_set_byte(snd_ctl_elem_value_t *obj, unsigned int idx, un
> >>>> void snd_ctl_elem_set_bytes(snd_ctl_elem_value_t *obj, void *data, size_t size)
> >>>> {
> >>>> assert(obj);
> >>>> - if (size >= ARRAY_SIZE(obj->value.bytes.data)) {
> >>>> - assert(0);
> >>>> - return;
> >>>> - }
> >>>> + assert(size < ARRAY_SIZE(obj->value.bytes.data));
> >>>> memcpy(obj->value.bytes.data, data, size);
> >>>> }
> >>>>
> >>>> --
> >>>> 2.5.0
> >>>>
> >>> _______________________________________________
> >>> Alsa-devel mailing list
> >>> Alsa-devel@xxxxxxxxxxxxxxxx
> >>> http://mailman.alsa-project.org/mailman/listinfo/alsa-devel
> >>>
>
_______________________________________________
Alsa-devel mailing list
Alsa-devel@xxxxxxxxxxxxxxxx
http://mailman.alsa-project.org/mailman/listinfo/alsa-devel
