[Dune] RFC 01/2011: replace row_type & by row_reference in DenseMatrix

[Martin Nolte]

Hi Oli,

providing the information is no problem at all.

However, I did not consider this an interface change and, therefore, did 
not see any reason to bother everybody with this small change. For the 
user, everything looks exactly the same. The only "interface" classes I 
touched are FieldMatrix and DynamicMatrix, which still behave exactly as 
they did before. In my opinion, the DenseMatrix is just an 
implementational detail. Maybe my judgement was different from others on 
this point.

Is there some way of avoiding such misunderstandings in the future? For 
example, we could write a comment like "THIS IS AN INTERFACE CLASS" 
above each class that should not be changed without notifying the other 
developers.

Best,

Martin

On 10/14/2011 05:47 PM, Oliver Sander wrote:
> Hi Martin!
> Thanks for information, even if it's after the fact. As I said I don't
> really have any objections to your commit itself, only to the autocratic
> way of deciding on interface changes. Let's see if there are more comments.
>
> cheers,
> Oliver
>
> Am 14.10.2011 15:03, schrieb Martin Nolte:
>> Dear all,
>>
>> I have replaced the return value of the operator[] of a DenseMatrix
>> (which was row_type &) by a row_reference, which is exported from the
>> MatVecTraits. For the corresponding matrix implementations, i.e.,
>> FieldMatrix and DynamicMatrix, I added the corresponding typedefs
>> (row_reference and const_row_reference) to their MatVecTraits.
>>
>> This change has very little impact on your current code, as no types are
>> actually changed, but allows you to build a DenseMatrix that has a
>> single FieldVector as container for its values.
>>
>> Despite the heavy use of FieldMatrix in the core modules, few lines
>> actually obtain the row_type from the FieldMatrix and would have to be
>> changed. Actually, the word row_type does not occur in any header of
>> dune-grid or dune-localfunctions, but only in dune-common and dune-istl.
>> In dune-common, the only place I did not change is exprtmpl.hh, but I
>> guess this is not a big problem. In dune-istl, there are only two lines
>> actually using row_type &, namely:
>>
>> diagonalmatrix.hh:575:
>> row_ = row_type(&(mat_->diagonal(i)), i);
>>
>> io.hh:441:
>> const typename MatrixType::row_type& row = matrix[rowIdx];
>>
>> So nearly all current code makes use of explicit knowledge of what
>> row_type and, hence, row_type &, actually is. Therefore, I think the
>> only files that might need to be changed are those mentioned. However,
>> for dune-istl, this might break code for other kinds of matrices that do
>> not, yet, support a row_reference. Maybe we have to add this to
>> dune-istl, too?
>>
>> Since there has already been some discussion on this, I would like to
>> hear your comments, opinions, and suggestions. It would also be great to
>> know whether someone objects.
>>
>> As the change is not very intrusive to existing codes, I will leave the
>> commit in place until the discussion is over. I hope there are no
>> objections to this.
>>
>> Best,
>>
>> Martin
>>
>>
>>
>> PS: There you go Oli. Maybe I slightly went past the goal. However, I
>> dislike writing around 100 lines to explain a change of around 20 lines.
>> Somehow the gain does not balance the cost. Unfortunately, I do not have
>> a better solution to the communication problem, either.
>>
>> _______________________________________________
>> Dune mailing list
>> Dune at dune-project.org
>> http://lists.dune-project.org/mailman/listinfo/dune
>
> _______________________________________________
> Dune mailing list
> Dune at dune-project.org
> http://lists.dune-project.org/mailman/listinfo/dune