Set real_type of MultiTypeBlockVector

Summary

The MultiTypeBlockVector already defines its field_type conditionally as common-type over all field-types of its components. If no such common-type exists, the field_type is defined as Std::nonesuch. In this case, the real_type shouldn't simply be defines as using real_type = typename FieldTraits<field_type>::real_type, since then the FieldTraits be potentially illformed or by default the same as Std::nonesuch. While there might not be a common field_type it could still be defined a common real_type.

This MR fixes this in MultiTypeBlockVector and correspondingly in MultiTypeBlockMatrix by defining field_type and real_type independently.

mentioned in merge request dune-common!1040

resolved all threads

approved this merge request

changed the description

added 177 commits

• ccacfa01...20bcffa6 - 174 commits from branch master
• 5fcbfc75 - Set real_type of MultiTypeBlockVector
• 09a47cbd - Set field_type and real_type correctly in MultiTypeBlockMatrix
• 9d3eb180 - Remove old definitions of field_type and real_type

Compare with previous version

changed title from WIP: Set real_type of MultiTypeBlockVector to Set real_type of MultiTypeBlockVector

added 1 commit

• 5bf1a5c6 - Set field_type and real_type correctly in MultiTypeBlockMatrix

Compare with previous version

added 1 commit

• 1914d7f1 - Add changelog entry

Compare with previous version

added 1 commit

• 9966375f - Add changelog entry

Compare with previous version

enabled an automatic merge when the pipeline for 9966375f succeeds

mentioned in commit f37f90ae

merged

This breaks downstream code: Before this MR it was OK, to use matrix type providing ::field_type, but not specializing FieldTraits. This MR breaks downstream code by making the specialization of FieldTraits mandatory.

I don't understand, it should have been mandatory before, because the FieldTraits<Args...>::field_type was used before. This MR extends this to real_type and also in the MultiTypeBlockMatrix that has used the MultiTypeBlockVector::field_type before and thus also the FieldTraits specialization.

The annoying thing about this, is that you can no longer simply derive from a dune-istl matrix, because the derived matrix does not inherit the FieldTraits specialization.

I strongly suggest to no longer promote the use of FieldTraits until we have fixed its behavior and added a precise definition. If this incorporates that we introduce a breaking change, this should be clearly documented in the changelog. Currently the entry is wrong in this respect, because it gives the impression that a common type for the M::field_type types should exist. In contrast to this the implementation requires a common type of the FieldTraits<M>::field_type types.

I don't understand, it should have been mandatory before, because the FieldTraits<Args...>::field_type was used before. This MR extends this to real_type and also in the MultiTypeBlockMatrix that has used the MultiTypeBlockVector::field_type before and thus also the FieldTraits specialization.

It was used in some of the template methods which may or may not be instantiated in user code. Now it is a hard requirement that is enforced by a static_assert.

No, I mean the line MultiTypeBlockVector:82

using field_type = Std::detected_t<std::common_type_t, typename FieldTraits< std::decay_t<Args> >::field_type...>;

was there before. With a static_assert.

This MR was merged to fix the issues showing up in dune-common!1040. Sorry, that I broke downstream code. I have tested many modules but no problem showed up.

This enforces, that the entries of the MultiTypeBlockVector specialize FieldTraits. This MR enforces that the entries of MultiTypeBlockMatrix specialize this. That's a difference. The downstream failure happens, if you use a class derived from MultiTypeBlockVector as entry.

Ah, now I see. But this is outside of the scope of the MultiTypeBlockMatrix. It states "This matrix class combines MultiTypeBlockVector elements as rows." At least this was my assumption.

All these changes require proper specializations of FieldTraits. If this is now mandatory, we should help users to notice this.

E.g. we could add to all matrix and vector types a static assertion that ensures FieldTraits<T>::field_type != T whenever IsNumber<T> is not true.

Ah, now I see. But this is outside of the scope of the MultiTypeBlockMatrix. It states "This matrix class combines MultiTypeBlockVector elements as rows." At least this was my assumption.

That's a good argument. However, one may argue that "B derives from A" implies that each instance of B is also an instance of A.

How to proceed? We could either revert the MR and go back to the old state. This is in conflict with the dune-common!1040. Or we find a compatible solution.

Again, I can fix my downstream code. But I'm afraid that this may happen to users less experienced with the field_type-mess, because the errors that you may get once the broken default for FieldTraits kicks in are really nasty.

One solution I could think of: I don't use FieldTraits specializations of the rows in the MultiTypeBlockMatrix but just the ::field_type member, since I would assume that the rows are of MultiTypeBlockVector those should exist. And I do a common_type over these direct aliases, instead of the FieldTraits indirection.

This should not make the situation worse than before, but still solves the real_type issue that shows up when we fix the FieldTraits as suggested in dune-common.

Maybe I could improve the static_assert. It was supposed to tell the user of the class to provide a FieldTraits specialization of the block types.

We could also go back to the old BlockTraits. Unfortunately, there is no real_type. In the multi-type-block-vector case if no common field_type exists, we cannot use the FieldTraits to produce a real_type.