Fix for a compiler warning

[davidedelvento]

Component DIPlib at the latest version of the main branch (at time of writing) namely f9c6e37939e6f7917943cec400442ce6e5bcb2af

Describe the bug At compile time, there are two warnings, one of which is

In file included from /usr/include/c++/11/algorithm:61,
                 from /home/davide/repo/github/diplib_davide/src/library/image_data.cpp:20:
In function ‘typename __gnu_cxx::__enable_if<std::__is_scalar<_Tp>::__value, void>::__type std::__fill_a1(_ForwardIterator, _ForwardIterator, const _Tp&) [with _ForwardIterator = long unsigned int*; _Tp = long unsigned int]’,
    inlined from ‘void std::__fill_a(_FIte, _FIte, const _Tp&) [with _FIte = long unsigned int*; _Tp = long unsigned int]’ at /usr/include/c++/11/bits/stl_algobase.h:969:21,
    inlined from ‘void std::fill(_ForwardIterator, _ForwardIterator, const _Tp&) [with _ForwardIterator = long unsigned int*; _Tp = long unsigned int]’ at /usr/include/c++/11/bits/stl_algobase.h:999:20,
    inlined from ‘void dip::DimensionArray<T>::resize(dip::DimensionArray<T>::size_type, T) [with T = long unsigned int]’ at /home/davide/repo/github/diplib_davide/include/diplib/library/dimension_array.h:216:25,
    inlined from ‘void dip::DimensionArray<T>::pop_back() [with T = long unsigned int]’ at /home/davide/repo/github/diplib_davide/include/diplib/library/dimension_array.h:325:16,
    inlined from ‘void dip::Image::MatchStrideOrder(const dip::Image&)’ at /home/davide/repo/github/diplib_davide/src/library/image_data.cpp:300:19:
/usr/include/c++/11/bits/stl_algobase.h:924:18: warning: ‘void* __builtin_memset(void*, int, long unsigned int)’ specified size 18446744073709551608 exceeds maximum object size 9223372036854775807 [-Wstringop-overflow=]
  924 |         *__first = __tmp;
      |         ~~~~~~~~~^~~~~~~

To Reproduce

cmake <path_to_diplib_repository>
make -j check

System information:

• Ubuntu 22.04.4 LTS
• gcc (Ubuntu 11.4.0-1ubuntu1~22.04) 11.4.0

Fix Not sure if this would go away with a more recent version of the compiler (didn't get a chance to try it yet), but to wet my toes into the DIPlib development environment, I tracked this down to gcc not recognizing the DIP_ASSERT protecting the feared case from happening (more below).

This is easily avoided for example with a patch like the following. Such a change, obviously, is acceptable only assuming that this code is not exercized too often to make the additional check a measurable performance penalty. Moreover, that is redundant with the already present ASSERT, hence I'm presenting this as a bug to discuss, rather than a PR. Happy to make a PR if that's an acceptable change and deemed like the best way to proceed.

--- a/include/diplib/library/dimension_array.h
+++ b/include/diplib/library/dimension_array.h
@@ -322,7 +322,9 @@ class DIP_NO_EXPORT DimensionArray {
       /// Removes the value at the back.
       void pop_back() {
          DIP_ASSERT( size_ > 0 );
-         resize( size_ - 1 );
+    if (size_ > 0) {
+        resize( size_ - 1 );
+    }
       }

If my understanding is correct, what is confusing gcc is that DIP_ASSERT is throwing rather than "avoid execution". In fact the following fails in the same way as the original code, even though it would never reach the offending line. This used to be a problem in very old versions of the compiler, but I was actually surprised to see it reappear today -- maybe there is some compiler option which is forcing this behavior?

index a37852a0..8a24681c 100644
--- a/include/diplib/library/dimension_array.h
+++ b/include/diplib/library/dimension_array.h
@@ -322,6 +322,9 @@ class DIP_NO_EXPORT DimensionArray {
       /// Removes the value at the back.
       void pop_back() {
          DIP_ASSERT( size_ > 0 );
+        if (size_ == 0) {
+                throw;
+        }
          resize( size_ - 1 );
       }

PS: regardless, I think that fixing these two remaining warnings is important. In fact I greatly appreciate the very clean build environment of DIPlib without lots of "accepted" warnings cluttering the build log and potentially hiding issues when contributing -- so thanks for making DIPlib such a pleasant library to dive into.

[crisluengo]

Davide, thank you so very much!

I have indeed seen this compiler warning, for me it fired only in the tests for DimensionArray<>. It's obviously a bad diagnostic, since running those tests under Adress Sanitizer shows no issues.

I don't see an issue with adding the if (size_ > 0) test, pop_back is used only a few times throughout the library, and never called more than once in a row (the DimensionArray<> is not suited for repeated pushing or popping). But please remove the assert, which will no longer be useful.

[davidedelvento]

My pleasure Cris! I agree that would the compiler be better this would not be necessary, but as I mentioned keeping the build log clean makes it easier to spot issues before they become problems in production, so I appreciate going with the "less evil" so to speak. Made #154 as you suggested and looking forward to more meaningful contributions.