Remove unreachable code from GE5ImageIO and use unique_ptr

[N-Dekker]

GE5ImageIO::ReadHeader appeared to have unreachable code in if-clauses like this:

buffer = new char[N];
if (buffer == nullptr)
{
  f.close();
  itkExceptionMacro("GE5ImageIO:Unable to allocate memory...!");
}

This pull request proposes to remove those unreachable lines of code, and replace locally declared raw pointers by unique_ptr, in GE5ImageIO.

[jhlegarreta]

@N-Dekker Thanks for all these efforts to avoid the use raw pointers.

In this case, the rationale and/or commit message looks to be inconsistent with the coverage report:
https://open.cdash.org/viewCoverageFile.php?buildid=8157810&fileid=52192360

The "unreachable code" is actually being reached.

Any ways to explain why that is happening if the code should be unreachable?

[N-Dekker]

In this case, the rationale and/or commit message looks to be inconsistent with the coverage report: https://open.cdash.org/viewCoverageFile.php?buildid=8157810&fileid=52192360

The "unreachable code" is actually being reached.

Any ways to explain why that is happening if the code should be unreachable?

Very interesting @jhlegarreta ! Those lines that say itkExceptionMacro("GE5ImageIO:Unable to allocate memory... can't possibly be reached, because new char[N] never ever returns null, by definition! Could it be a bug in the Coverage analysis?

[N-Dekker]

From a very old (but pretty famous) C++FAQ, by Marshall Cline:

Do I need to check for NULL after p = new Fred()?
No! (...)

https://www.cs.technion.ac.il/users/yechiel/c++-faq/new-never-returns-null.html

[issakomi]

Just as a joke, not applicable.

https://www.cs.technion.ac.il/users/yechiel/c++-faq/new-never-returns-null.html

Fred* p = new Fred();
if (p == NULL)    ← only needed if your compiler is from the Stone Age!
  throw std::bad_alloc();

Hmm, we don't know here what is class Fred, may be e.g.:

#include <iostream>

class Fred
{
public:
  void * operator new(size_t) /* throw() */
  {
    return nullptr;
  }
  void operator delete(void*)
  {
  }
};

int main(int, char**)
{
  Fred * f = new Fred;
  if (f == nullptr)
  {
    std::cout << "Fred is nullptr" << std::endl;
  }
  delete f;
  return 0;
}

r@deb2:~$ ./a.out 
Fred is nullptr

Works for me with and without throw(), without GCC 12.1.0 shows warning.

Edit:
BTW, i have checked ITK code for overloaded operator new, Eigen uses this technique to align memory for SIMD,
s. Memory.h. Just FYI, related to bulk replacements with make_unique_for_overwrite in general, not for the current PR.

[N-Dekker]

Hmm, we don't know here what is class Fred, may be e.g.: [...]

@issakomi Very clever 👍, Mihail! Thanks! Now I just hope we can keep your Fred out of the ITK source tree 😸

Just FYI, related to bulk replacements with make_unique_for_overwrite in general, not for the current PR.

Indeed, not relevant for this specific PR, but itk::make_unique_for_overwrite does take user-defined operator new[] overloads into account, as I just checked:

class Flintstone
{
public:
  void *
  operator new[](size_t)
  { return nullptr;
  }
  void
  operator delete[](void *)
  {}
};


TEST(MakeUniqueForOverwrite, UniqueFlintstoneIsNull)
{
  const auto uniqueFlintstone = itk::make_unique_for_overwrite<Flintstone[]>(42);
  ASSERT_EQ(uniqueFlintstone, nullptr); // Passes successfully (no assert failure).
}

So I think it works as expected. Right?

[jhlegarreta]

Could it be a bug in the Coverage analysis?

IDK. I have observed at times coverage reports that seemed misleading (e.g. not reporting lines as covered when the test contained calls to them, and the file was contained in the CMakeLists.txt file).

In this case, a way to see if those lines are actually hit is to inspect the output of the corresponding test in the dashboard.

Also, I remember reading in other PRs compilers throwing warnings about unreachable code (maybe GCC 12.1.0 as @issakomi mentions?).

[issakomi]

Тhe class Fred was a joke of course, sorry for the noise ... But it is good that Eigen's scenario with overloaded new was checked, better check twice, IMHO.

compilers throwing warnings about unreachable code

IMHO, i don't think compiler will always warn about dead code, i have discovered several such bugs in my code with Synopys Coverity analyzer, compiler said nothing.

#include <iostream>
int main(int, char**)
{
	const bool b = true;
	int a = 1;
	if (!b)
	{
		++a;
	}
	std::cout << a << std::endl;
	return 0;
}

r@deb2:~$ g++ test.cpp 
r@deb2:~$ g++ -Wall test.cpp
r@deb2:~$ g++ -O2 -Wall test.cpp 
r@deb2:~$ 

No warning.

[N-Dekker]

No warning.

Interesting... for what it's worth, the following does at least produce a warning from MSVC, saying:

(10): warning C4127: conditional expression is constant

int main()
{
	const bool b = true;
	int a = 1;
	if (b == false)
	{
		++a;
	}
	return a;
}

At https://godbolt.org/z/E69ea4Yer

[N-Dekker]

For the record, the original if ( buffer == NULL ) checks were introduced with commit fea55fa in 2010. The very first edition of the C++ Standard (1998) already specified that expressions like new char[N] "only indicate failure by throwing
an exception", not by returning a null pointer.

Anyway, thanks for your comments @issakomi, @jhlegarreta and thanks for your approval @dzenanz I think this pull request is ready 😃

[issakomi]

        int* p = new(std::nothrow) int[100000000ul]; // non-throwing overload
        if (p == nullptr)

another alternative, but just for completeness, not applicable for the PR
https://en.cppreference.com/w/cpp/memory/new/nothrow
https://cplusplus.com/reference/new/nothrow/

[N-Dekker]

Thanks for merging @hjmjohnson Of course, it would have been nice if any static analysis tool would have produced a warning on unreachable code! Moreover, I find it really misleading that ubuntu2004 Coverage 8 claimed that those unreachable lines of code were actually covered. At https://open.cdash.org/viewCoverageFile.php?buildid=8157810&fileid=52192360 as remarked by @jhlegarreta. Hope the Coverage tool will still be fixed!