Re: [[patch] valarray (was Re: valarray_copy implementation question)

[Gabriel Dos Reis <gdr at integrable-solutions dot net>]

On Thu, Nov 26, 2009 at 2:19 AM, Christian BRUEL <christian.bruel@st.com> wrote:
> Hello Gabriel,
>
> my answers :
>
> Q#1: "Why is this change necessary?"
>
> Cosmetic consistency reflex only to make all the calls to
> std::__valarray_copy look similar. Since the coding convention allow lines
> of 80 chars I just removed the unnecessary line break while editing the
> file. This is not strictly mandatory I agree.

Since your patch does not appear correct to me,I would like to
concentrate only on the changes that matter.

>
> Q#2,#3 "Why change from __v.size() to _M_sz?"
>
> These are the only important changes of the patch.

In that case, I believe the patch is incorrect.

> The way that _valarray_copy is implemented is to look up thru the number of
> true elements, so it was wrong to bound the scan to the number of arrays
> elements, which inevitably lead to my outofbound reports if the n of true
> elements was smaller that the size of the array.

In that case, the data violate the preconditions of the library.

> Now we just need to scan and iterate n true elements time.
> On a performance point of view, there are no reason to continue a scan of
> the array once the last true element as been processed.

Then we would be implementing a different thing.

>
> Q#4 "myarray[mymask] += valarray<int>(3,5);
> ? I'm not sure I understand this line.
> ? The way I read it, the left hand side is not of length 5.
> ? Is that correct?"
>
> No, in this line the left hand size has size of n=10. The right hand side
> has size of 5.

That makes the assignment have an undefined behaviour.

> But I fixed this line to be
> ? myarray[mymask] += valarray<int>(3,n);
> so both sides have the same size, unrelated to the number of true elements
> in mymask.

No.  Both sides don't have the same number of elements.  The left hand
side has size 0, and n is 10.  I don't see how you equate 0 to 10.

-- Gaby