Feature/2dfunction part2

[whaeck]

Adding x,f(y) tabulated data and tests using LegendreSeries and InterpolationTable.

[joewpeterson]

I'm having to do some reading on placement new as I'm not very familiar with them... Is the destructor going to be called on the object when it goes out of scope?

Also ... I'm having trouble working out how it is possible that this is a good idea given the construction of InterpolationTableFunction

What are we doing? If base not this we put base into the memory location of this, yes?

does base have the same types as this? If not, there could be a problem, say, if this is made of floats and base is made of doubles.

[whaeck]

There is no need to call the destructor in this case since the destructor is a trivial one (i.e. if the class had a custom one, we would have to use it). I generally dislike using raw pointers which would require using a custom destructor.

InterpolationTable and InterpolationTableFunction have an internal vector of TableVariant which represent the various interpolation regions using light-weight iterator based views that point to the actual x and y values. These internal components are used for table evaluation, linearisation, etc. One of the major annoyances one can encounter in tabulated data are discontinuities in the x grid. By using these interpolation regions represented by a TableVariant, we can ensure there are no discontinuities within the region (they can only occur on the edges of a region) and that every x value only occur once.

We have to declare non-default copy and move constructors and assignment since we need to ensure these internal iterators are always consistently pointing to their data (if we rely on the default constructor, the internal iterators after copying would still point to the original tables's x and y array instead of the copied one). While iterators are basically pointers, they do not point to memory that we need to deallocate (the vectors they point to take care of that). The general technique used here is to inplement the copy and move constructor and implement the copy and move assignment using new placement with the copy and move constructor.

if ( this != &base ) is a safeguard against doing self-assignment (i.e. writing table = table;). That would be bad.

base is always the same type as the class for which we declare the copy and move constructor and assignment operators. Because of the way we implement these, we cannot rely on defaults for these constructors and operators on derived types. Derived types (e.g. dryad's TabulatedCrossSection, which derives from InterpolationTable) need to have their own copy and move constructor and assignment operators because otherwise we'd be returning an instance of InterpolationTable or InterpolationTableFunction instead of the derived type.

[joewpeterson]

Thanks for the explanation. I'm going to have to do some playing with this to understand the dangers a little bit better.

At the moment I'm not certain I agree about the destructor. Here is what I'm thinking: A and B are independent, fully defined class of this type. Someone calls A = B. What happens? The contents of B are put into the address of A. How can the code call the destructor on A anymore? I think not - what it would destroy now is no longer referenced at the expected memory locations. A had vectors in it. Have those vectors gone out of scope as a result of copying over their memory locations? I think not. That sounds like a memory leak to me. If this is strictly used like
A = new (sizeof class)
A = B
I see no issue. But is that a use restriction we guarantee?

As for the classes needing to have the same type... is that true? The class is templated. with a function inputs defined as
InterpolationTableFunction& operator=( const InterpolationTableFunction& base ), I think the RHS of
A = B
need only be a const InterpolationTableFunction&. Does it need to be the same InterpolationTableFunction& type as the LHS? Certainly I can write a function which returns a vector of floats which takes in a vector of doubles. If A is made up of all floats and B is made up of all doubles, It is not obvious to me that this operator would result in an error - And that makes me think of memory corruption possibilities, not something which would necessarily be caught with tests.

I'll have to consider this more to prove to myself these are unfounded fears

[joewpeterson]

I had some time to investigate this a little more this afternoon and I'd like to follow-up On the concern of memory corruption: this is unfounded. C++ compiler for the operator= does insist on a conversion existing for the RHS and LHS.

However, the memory leak concern does seem valid. Testing the following code in Valgrind memory checker does find one leak. A leak detection of analysis where Entity only has an int member found no memory leak. So the story I think is happening: B gets copied into A. Old A's destructor now cannot be executed on Entity Old A because all trace of its existence in memory has been overwritten. Old A had reference to vectors which did have pointers to heap allocation. Because no destructor call on Old A is possible, those vectors don't go away and one leaks memory.

1   template <typename X>
  2 class Entity{
  3         std::vector<X> x_;
  4 public:
  5         Entity(X x){
  6                 x_.push_back( x );
  7         }
  8 
  9         Entity& operator=( const Entity& base ){
 10                 new (this) Entity(base);
 11                 return *this;
 12         }
 13 };
 14 
 15 int main()
 16 {
 17         Entity A(2);
 18         Entity B(4);
 19         A = B;
 20 
 21         return 0;
 22 }

[joewpeterson]

In discussion with Wim, we've decided to go ahead with the merge despite this memory leak issue. A new task will be created (#79 ) to fix this as well as any other cases where a placement new may cause a memory leak.