Fix the view memory leak and array/view copy/move.

[tcojean]

This fixes some of the discussed issues with arrays. The changes are noted in the list below. Some of these points should still be discussed.

• Add tests for copy/move of all combinations of array/view.
• Add a new exception helper macro for testing array dimensions.
• When copying something into a view, do bound checking and ensure what is
  copied has a compatible amount of data.
• When copying into an array, the proper amount of data is allocated and copied
  over.
• When moving a into b, b becomes a and a is invalid afterwards.
• Resize and reset for views throws an error.

[upsj]

LGTM, mainly a few suggestions for more restrictive tests.

Concerning your open questions, I agree with most of them, but tend to disagree about the resize_and_reset. In my opinion, it should either turn a view into an array if needed, or set the size to 0 instead of leaving it intact, when the data pointer is a null pointer.

I believe there are two ways we can evaluate the different approaches you presented: By expectations (which behavior would someone intuitively expect) or by convenience (which behavior makes it easiest for us to implement our algorithms, i.e. needs the smallest amount of hacks, workarounds etc.)

[codecov]

Codecov Report

Merging #485 into develop will decrease coverage by 0.05%.
The diff coverage is 100.00%.

@@             Coverage Diff             @@
##           develop     #485      +/-   ##
===========================================
- Coverage    88.86%   88.80%   -0.06%     
===========================================
  Files          256      256              
  Lines        16484    16546      +62     
===========================================
+ Hits         14648    14694      +46     
- Misses        1836     1852      +16     

Impacted Files	Coverage Δ
include/ginkgo/core/base/exception_helpers.hpp	91.66% <ø> (ø)
core/test/base/array.cpp	100.00% <100.00%> (ø)
include/ginkgo/core/base/array.hpp	96.66% <100.00%> (+0.23%)	⬆️
omp/components/format_conversion.hpp	40.00% <0.00%> (-60.00%)	⬇️
include/ginkgo/core/matrix/ell.hpp	68.75% <0.00%> (-31.25%)	⬇️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 9435b45...ad9802b. Read the comment docs.

[pratikvn]

LGTM! Only some minor things.

[pratikvn]

Regarding when copying into a view, bounds checking and a throw looks reasonable to me.

The resize_and_reset gets called when moving from an view, right ? In that case, intuitively, to me, setting the pointer to nullptr and not allocating makes sense. I am not sure if it makes sense to decide based on whether something makes implementing our algorithms easier because if the function does something un-intuitive, then using and debugging it becomes a lot harder.

[thoasm]

Very nice extensive tests, I have some minor comments on them.

I would like to discuss what we intend to do with custom deleter because they could also be a potential problem (can be a potential data leak in the current form).
I am in favor of treating all non-default_deleter as views. That would:

• (potentially) prevent memory leaks (simply by preventing allocations for non-default_deleters) and
• make it easier to detect views (and similar)

If we do not allow resizing of views, we might need means to detect if an Array is a view or not (See @upsj example with ILUT or SPGEMM). A public helper function might be useful here.

[yhmtsai]

I have a question about what's happened if we set_executor on a view?
does it become an array?

[thoasm]

LGTM!

[thoasm]

@tcojean Can you add a test with a custom deleter and copying of views with mismatching sizes?
That way, we document that this is a potential problem we need to address.

ASSERT_THROW(view1_custom_del = view2, gko::NotSupported);

[tcojean]

Copying view test with mismatched size is already there I think

ASSERT_THROW(view2 = view_size4, gko::OutOfBoundsError);

[tcojean]

Anyone has any idea why I get this weird Windows compilation issue? I don't think I changed anything relevant...?

D:\a\ginkgo\ginkgo\core/base/iterator_factory.hpp(297,2): error C2398: Element '2': conversion from 'size_t' to 'gko::detail::IteratorFactory<IndexType,ValueType>::Reference::array_index_type' requires a narrowing conversion [D:\a\ginkgo\ginkgo\build\reference\ginkgo_reference.vcxproj]
          with
          [
              IndexType=gko::int64,
              ValueType=std::complex<double>
          ]
D:\a\ginkgo\ginkgo\core/base/iterator_factory.hpp(296): message : while compiling class template member function 'gko::detail::IteratorFactory<IndexType,ValueType>::Reference gko::detail::IteratorFactory<IndexType,ValueType>::Iterator::operator [](size_t) const' [D:\a\ginkgo\ginkgo\build\reference\ginkgo_reference.vcxproj]
          with
          [
              IndexType=gko::int64,
              ValueType=std::complex<double>
          ]
C:\Program Files (x86)\Microsoft Visual Studio\2019\Enterprise\VC\Tools\MSVC\14.25.28610\include\algorithm(2749): message : see reference to function template instantiation 'gko::detail::IteratorFactory<IndexType,ValueType>::Reference gko::detail::IteratorFactory<IndexType,ValueType>::Iterator::operator [](size_t) const' being compiled [D:\a\ginkgo\ginkgo\build\reference\ginkgo_reference.vcxproj]
          with
          [
              IndexType=gko::int64,
              ValueType=std::complex<double>
          ]

[upsj]

#490 simply rebase onto develop

[tcojean]

Thanks... I thought I was already up to date with develop but I guess I missed this one.

[sonarcloud]

Kudos, SonarCloud Quality Gate passed!

0 Bugs
0 Vulnerabilities (and 0 Security Hotspots to review)
5 Code Smells

15.4% Coverage
0.0% Duplication

[pratikvn]

LGTM!

[yhmtsai]

LGTM