#892 FieldVector-FieldVector operations must statically check the size!

Metadata

Property	Value
Reported by	Markus Blatt (markus@dr-blatt.de)
Reported at	Mar 3, 2011 19:55
Type	Bug Report
Version	Git (pre2.4) [autotools]
Operating System	Unspecified / All
Last edited by	Markus Blatt (markus@dr-blatt.de)
Last edited at	Mar 4, 2011 09:54

Description

There a quite a few methods in FieldVector with a FieldVector as the only argument, that only make sense if both vectors have the same size. Unfortunately, most of them are forwarded to the base class DenseVector which only has a dynamic assertion which checks the size.

This is far from ideal and I would propose a static_assert in FieldVector itself!

I have already fixed this for the copy constructor.

Some of the methods that should still be changed are:

operator=
operator+=
operator-=

...

shailesh borkar (shaileshborkar9@gmail.com) commented on the issue:

Hello Markus sir, In FieldVector class, there is only one method (the constructor) that takes single argument of type FieldVector and you have already fixed it for static_assert, but the other methods operator= operator+= operator-= are not there in FieldVector but in DenseVector. So how one should go for applying static_assert to these methods in FieldVector itself?

Tilak Raj Singh (tilak72@gmail.com) commented on the issue:

Hi Shailesh, I think static_assert to these methods in FieldVector itself can be applied by defining these functions in fvector.hh. As for field vectors the compiler will first search for functions in child fvector if not found then it will go to its parent densevector. I propose the following .diff file.

Attachments

• static_assert.diff

shailesh borkar (shaileshborkar9@gmail.com) commented on the issue:

Hi Tilak, Won't it be a redundant code after adding the same functions in child class. What I am not able to understand here is, why does Markus sir want static_assert in FieldVector itself? Because if this has to be done for fvector, then why not for the other classes also. There will be no reusability.

Tilak Raj Singh (tilak72@gmail.com) commented on the issue:

thats bcz densevector has dynamic assert and it would be used for other classes having it as its parent. Its just a specialization for fvector

@tilak I agree with shailesh that this would be a lot of redundant code. Unless there is a good reason for it (e.g. better performance), we should avoid code duplication for better maintainance. BTW DenseVector can have static or dynamic size.

Tilak Raj Singh (tilak72@gmail.com) commented on the issue:

then perhaps we can use static assert in densevector itself???

Was this fixed by @pipping's DUNE_ASSERT_BOUNDS fix for #1030 (closed), too? What's about the

assert(x.size() == SIZE); // Actually this is not needed any more!

in the code? If this is really not needed, why wasn't it removed?

@gruenich: The cases mentioned here, like

    Dune::FieldVector<double, 3> v1 = {1, 2, 3};
    Dune::FieldVector<double, 2> const v2 = {1, 2};
    auto x = v1 + v2;

are covered by DUNE_ASSERT_BOUNDS (although this is apparently not tested anywhere). This bug report remains valid, though, because the issue could be detected at compile-time already, and currently is not.

Update: Since operator+, operator-, etc. are implemented such that they create a temporary and apply operator+=, operator-=, etc., there's no point in adding more tests.

@gruenich As for the

assert(x.size() == SIZE); // Actually this is not needed any more!

It's needed and not needed at the same time.

If a FieldVector is constructed from an initialization list, a for-loop that assigns entries of one to the other could terminate once i < some_size where some_size could be the length of the FieldVector or the initialization_list. When I implemented that, I just went with one of them and put an assertion in place that makes sure they're the same so that

    Dune::FieldVector<double, 3> v1 = {1, 2, 3};

is legal but

    Dune::FieldVector<double, 3> v1 = {1, 2}; // case 1

is not and neither is

    Dune::FieldVector<double, 3> v1 = {1, 2, 3, 4}; // case 2

This was later changed (not by me) such that the minimum of both sizes is taken. This would simply ignore the 4 in case 2 and leaves the third entry in case 1 unset. With that logic in place, it's no longer necessary to assure that both sizes are the same -- it's okay if they're not. By keeping the assertion around, we currently disallow both case 1 and case 2, though, so that while not strictly necessary, it still serves a purpose...

I am not a write of the static checks here. Reason: whenever such a check fails it is much more difficult to see why and fails and from where the call came from. In such cases, I routinely find myself replacing static_assert by assert and then gdb-ing into the assertion failure.

@gruenich As for that assertion again: It should be mentioned that e.g.

  char cr[3] = {'a', 'b', 'c', 'd'};

is not currently legal, so we might also not want that behaviour for Field* initialization.

I am in favor of not allowing wrong size for Field*.
I assume there is a use-case for a dim-sized FieldVector that get's three initial elements and ignores some elements for lower dimension. For me, this is still no reason to keep this error prone behavior.

@gruenich I second that the size of the initializer list should match exactly the size of the vector to be initialized.

There is only one exception: We traditionally allow Dune::FieldVector<double, 3>(1), which would initialize all members with 1.0. This can be very handy e.g. when initializing a YaspGrid. However, if we allow this, we also allow Dune::FieldVector<double, 3> f = { 1 };, I think. Which I am OK with; I just wanted to point out this case.

@joe: Actually, this doesn't seem to be a problem. The initializer list constructor takes precedence over uniform initialization (*). I tried it out, too:

  Dune::FieldVector<double, 3> x1 = {1, 2, 3}; // -> initializer list ctr
  Dune::FieldVector<double, 3> x2(1); // the old ctr
  Dune::FieldVector<double, 3> x3{1}; // -> initializer list ctr (assertion failure!)

(*) I can't be bothered to read the standard but that's what http://programmers.stackexchange.com/a/133690 says)

If you intend to use an initializer-list constructor, then you cannot statically reject

Dune::FieldVector<double, 3> x1 = {1, 2};

in C++11. (You might be able to pull it off C++14, though, since initializer_list::size() is constexpr there.)

Although @oliver.sander seems to dislike static checks anyway ;)

@joe: Oh, I didn't know that. That's great news. Huge fan of static assertions here :P