Code review comment for lp:~jobh/dolfin/fast-array

Revision history for this message
Garth Wells (garth-wells) wrote :

On 6 March 2012 15:13, Joachim Haga <email address hidden> wrote:
> After looking at la_get_set_items.i: _get_vector_values(), shown below, I
> think I'll punt on this. It's just too much work for an uncertain outcome
> (it is quite possible that a lot of complexity has to be added back to deal
> with cases like this, making this pointless).
>
> // Returns the values from a Vector.
> dolfin::Array<double>& _get_vector_values( dolfin::GenericVector* self)
> {
>  dolfin::Array<double>* values;
>  try
>  {
>    // Try accessing the value pointer directly [jobh: return borrowed data]
>    double* data_values = self->data();
>    values = new dolfin::Array<double>(self->size(), data_values);
>  }
>  catch (std::runtime_error e)
>  {
>    // We couldn't access the values directly [jobh: return owned data]
>    values = new dolfin::Array<double>(self->size());
>    self->get_local(*values);
>  }
>  return *values;
> }
>

We should just be able to use std::vector in the C++ interface for this.

Garth

>
> On 6 March 2012 15:01, Joachim Berdal Haga <email address hidden> wrote:
>
>> > I'm happy in principle, but want some clarity on Array. Can we make it
>>> > that Array is always a view (i.e., never owns that data), and
>>> > therefore cannot be resized? This would clean up some const hacks.
>>>
>>> I agree. For all argments needing to be resized should use
>>> std::vector. Not sure where the resize functionality is/was used.
>>
>>
>> It is possible (of course). There are quite a few user-exposed interfaces
>> that need changing (f.x. all the GeneralVector::set/get methods, since it
>> would be strange to just change the get methods).
>>
>> And it may be a lot of grunt work. But I'll have a go at it if you say
>> that's the way to go (even if it requires change to user code).
>>
>> -j.
>>
>>
>
> --
> https://code.launchpad.net/~jobh/dolfin/fast-array/+merge/94467
> Your team DOLFIN Core Team is requested to review the proposed merge of lp:~jobh/dolfin/fast-array into lp:dolfin.

« Back to merge proposal