Skip to content

Commit

Permalink
Remove copy/assign/move tests in testAttributes()
Browse files Browse the repository at this point in the history
When we implemented the "rule of 5" (PR #601) in 2019, I added code to
testAttributes.cpp to validate the behavior, specifically related to
the TypedAttribute class, but in retrospect, this code isn't reliable,
so I propose eliminating the test altogether.

The test counts invocations of the various constructors, destructors,
and move/assignment operators, expecting them to be consistent, but
compilers can optionally optimize some of these operations away,
producing inconsistent results.

In particular, a Debug build on Windows produces different results
from a Release build. Our CI has not historically tested a Windows
Debug build, which explains why we never caught this. The exact
behavior may have also changed with C++17. All the more reason to
avoid such picky tests.

This does not change the library code, only the tests.

Signed-off-by: Cary Phillips <[email protected]>
  • Loading branch information
cary-ilm committed Nov 19, 2024
1 parent e54d61b commit a336f55
Showing 1 changed file with 0 additions and 208 deletions.
208 changes: 0 additions & 208 deletions src/test/OpenEXRTest/testAttributes.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -567,212 +567,6 @@ print_type (const OPENEXR_IMF_NAMESPACE::TypedAttribute<T>& object)
cout << object.typeName () << endl;
}

OPENEXR_IMF_INTERNAL_NAMESPACE_HEADER_ENTER

//
// Test to confirm that the copy/move constructors are implemented
// properly.
//

static int default_constructor;
static int destructor;
static int copy_constructor;
static int assignment_operator;
static int move_constructor;
static int move_assignment_operator;

struct TestType
{
TestType () : _f (0) { default_constructor++; }

~TestType () { destructor++; }

TestType (const TestType& other) : _f (other._f) { copy_constructor++; }

TestType& operator= (const TestType& other)
{
assignment_operator++;
_f = other._f;
return *this;
}

TestType (TestType&& other) : _f (std::move (other._f))
{
move_constructor++;
}

TestType& operator= (TestType&& other)
{
move_assignment_operator++;
_f = std::move (other._f);
return *this;
}

static TestType func ()
{
TestType t;
return t;
}

static TestType arg (TestType a) { return a; }

int _f;

static void assert_count (int dc, int d, int cc, int ao, int mc, int mao)
{
assert (dc == default_constructor);
assert (d == destructor);
assert (cc == copy_constructor);
assert (ao == assignment_operator);
assert (mc == move_constructor);
assert (mao == move_assignment_operator);
}

static void reset ()
{
default_constructor = 0;
destructor = 0;
copy_constructor = 0;
assignment_operator = 0;
move_constructor = 0;
move_assignment_operator = 0;
}

static std::string str ()
{
std::stringstream s;
if (default_constructor)
s << "default_constructor=" << default_constructor << std::endl;
if (destructor) s << "destructor=" << destructor << std::endl;

if (copy_constructor)
s << "copy_constructor=" << copy_constructor << std::endl;

if (assignment_operator)
s << "assignment_operator=" << assignment_operator << std::endl;
if (move_constructor)
s << "move_constructor=" << move_constructor << std::endl;

if (move_assignment_operator)
s << "move_assignment_operator=" << move_assignment_operator
<< std::endl;
return s.str ();
}
};

typedef TypedAttribute<TestType> TestTypedAttribute;

template <>
const char*
TestTypedAttribute::staticTypeName ()
{
return "test";
}

template <>
void
TestTypedAttribute::writeValueTo (
OPENEXR_IMF_INTERNAL_NAMESPACE::OStream& os, int version) const
{}

template <>
void
TestTypedAttribute::readValueFrom (
OPENEXR_IMF_INTERNAL_NAMESPACE::IStream& is, int size, int version)
{}

TestType
testTypeFunc ()
{
return TestType ();
}

TestTypedAttribute
testFunc ()
{
TestTypedAttribute a;
return a;
}

TestTypedAttribute
testArg (TestTypedAttribute a)
{
return a;
}

OPENEXR_IMF_INTERNAL_NAMESPACE_HEADER_EXIT

void
testTypedAttribute ()
{
std::cout << "running testTypedAttribute()\n";

using namespace OPENEXR_IMF_INTERNAL_NAMESPACE;

//
// Validate the test type
//

TestType A;
TestType::assert_count (1, 0, 0, 0, 0, 0);
TestType::reset ();

TestType B (A);
TestType::assert_count (0, 0, 1, 0, 0, 0);
TestType::reset ();

B = A;
TestType::assert_count (0, 0, 0, 1, 0, 0);
TestType::reset ();

A = std::move (B);
TestType::assert_count (0, 0, 0, 0, 0, 1);
TestType::reset ();

A = TestType::func ();
TestType::assert_count (1, 1, 0, 0, 0, 1);
TestType::reset ();

A = TestType::arg (B);
TestType::assert_count (0, 2, 1, 0, 1, 1);
TestType::reset ();

//
// Test the attribute type
//

TestTypedAttribute a;
TestType::assert_count (1, 0, 0, 0, 0, 0);
TestType::reset ();

{
TestType x;
}
TestType::assert_count (1, 1, 0, 0, 0, 0);
TestType::reset ();

TestTypedAttribute b (a);
TestType::assert_count (0, 0, 1, 0, 0, 0);
TestType::reset ();

a = b;
TestType::assert_count (0, 0, 0, 1, 0, 0);
TestType::reset ();

a = std::move (b);
TestType::assert_count (0, 0, 0, 0, 0, 1);
TestType::reset ();

a = testFunc ();
TestType::assert_count (1, 1, 0, 0, 0, 1);
TestType::reset ();

a = testArg (b);
TestType::assert_count (0, 2, 1, 0, 1, 1);
TestType::reset ();

std::cout << "ok." << std::endl;
}

void
testAttributes (const std::string& tempDir)
Expand All @@ -781,8 +575,6 @@ testAttributes (const std::string& tempDir)
{
cout << "Testing built-in attributes" << endl;

testTypedAttribute ();

const int W = 217;
const int H = 197;

Expand Down

0 comments on commit a336f55

Please sign in to comment.