From ca98065949dee2f0dfd44ed2d2357111dc2f321a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Vladim=C3=ADr=20Vondru=C5=A1?= Date: Wed, 1 Jun 2022 09:53:26 +0200 Subject: [PATCH 1/3] TestSuite: create a non-templated base for Comparator. Majority of the work done in this class is template-independent, so this avoid a lot of needless template instantiations. --- src/Corrade/TestSuite/Comparator.cpp | 19 +++++++++++ src/Corrade/TestSuite/Comparator.h | 48 +++++++++++++++------------- 2 files changed, 44 insertions(+), 23 deletions(-) diff --git a/src/Corrade/TestSuite/Comparator.cpp b/src/Corrade/TestSuite/Comparator.cpp index 07f175590..7475c7b9b 100644 --- a/src/Corrade/TestSuite/Comparator.cpp +++ b/src/Corrade/TestSuite/Comparator.cpp @@ -27,6 +27,7 @@ #include "Comparator.h" #include "Corrade/Containers/EnumSet.hpp" +#include "Corrade/Containers/StringView.h" namespace Corrade { namespace TestSuite { @@ -57,4 +58,22 @@ Utility::Debug& operator<<(Utility::Debug& debug, const ComparisonStatusFlags va ComparisonStatusFlag::VerboseDiagnostic}); } +namespace Implementation { + +void ComparatorBase::printMessage(ComparisonStatusFlags, Utility::Debug& out, const char* const actual, const char* const expected, void(*printer)(Utility::Debug&, const void*)) { + CORRADE_INTERNAL_ASSERT(actualValue && expectedValue); + out << "Values" << actual << "and" << expected << "are not the same, actual is\n "; + printer(out, actualValue); + out << Utility::Debug::newline << " but expected\n "; + printer(out, expectedValue); +} + +/* LCOV_EXCL_START */ +void ComparatorBase::saveDiagnostic(ComparisonStatusFlags, Utility::Debug&, Containers::StringView) { + CORRADE_INTERNAL_ASSERT_UNREACHABLE(); +} +/* LCOV_EXCL_STOP */ + +} + }} diff --git a/src/Corrade/TestSuite/Comparator.h b/src/Corrade/TestSuite/Comparator.h index d0905006f..cda63f217 100644 --- a/src/Corrade/TestSuite/Comparator.h +++ b/src/Corrade/TestSuite/Comparator.h @@ -110,6 +110,23 @@ CORRADE_TESTSUITE_EXPORT Utility::Debug& operator<<(Utility::Debug& debug, Compa /** @debugoperatorenum{ComparisonStatusFlags} */ CORRADE_TESTSUITE_EXPORT Utility::Debug& operator<<(Utility::Debug& debug, ComparisonStatusFlags value); +namespace Implementation { + +class CORRADE_TESTSUITE_EXPORT ComparatorBase { + public: + explicit ComparatorBase(): actualValue{}, expectedValue{} {} + + void saveDiagnostic(ComparisonStatusFlags status, Utility::Debug& out, Containers::StringView path); + + protected: + void printMessage(ComparisonStatusFlags status, Utility::Debug& out, const char* actual, const char* expected, void(*printer)(Utility::Debug&, const void*)); + + const void* actualValue; + const void* expectedValue; +}; + +} + /** @brief Default comparator implementation @@ -202,10 +219,8 @@ In the above case, the message will look for example like this: @include testsuite-save-diagnostic.ansi */ -template class Comparator { +template class Comparator: public Implementation::ComparatorBase { public: - explicit Comparator(); - /** * @brief Compare two values * @@ -230,6 +245,8 @@ template class Comparator { */ void printMessage(ComparisonStatusFlags status, Utility::Debug& out, const char* actual, const char* expected); + /* Defined in the base class already */ + #ifdef DOXYGEN_GENERATING_OUTPUT /** * @brief Save a diagnostic * @@ -247,20 +264,10 @@ template class Comparator { * (and the function not being called at all if * @ref ComparisonStatusFlag::Diagnostic is not present as well). */ - #ifdef DOXYGEN_GENERATING_OUTPUT void saveDiagnostic(ComparisonStatusFlags status, Utility::Debug& out, Containers::StringView path); - #else - /* using const& to avoid having to include StringView.h */ - void saveDiagnostic(ComparisonStatusFlags status, Utility::Debug& out, const Containers::StringView& path); #endif - - private: - const T* actualValue; - const T* expectedValue; }; -template Comparator::Comparator(): actualValue(), expectedValue() {} - template ComparisonStatusFlags Comparator::operator()(const T& actual, const T& expected) { if(actual == expected) return {}; @@ -269,17 +276,12 @@ template ComparisonStatusFlags Comparator::operator()(const T& actua return ComparisonStatusFlag::Failed; } -template void Comparator::printMessage(ComparisonStatusFlags, Utility::Debug& out, const char* actual, const char* expected) { - CORRADE_INTERNAL_ASSERT(actualValue && expectedValue); - out << "Values" << actual << "and" << expected << "are not the same, actual is\n " - << *actualValue << Utility::Debug::newline << " but expected\n " << *expectedValue; -} - -/* LCOV_EXCL_START */ -template void Comparator::saveDiagnostic(ComparisonStatusFlags, Utility::Debug&, const Containers::StringView&) { - CORRADE_INTERNAL_ASSERT_UNREACHABLE(); +template void Comparator::printMessage(ComparisonStatusFlags status, Utility::Debug& out, const char* actual, const char* expected) { + Implementation::ComparatorBase::printMessage(status, out, actual, expected, + [](Utility::Debug& out, const void* value) { + out << *static_cast(value); + }); } -/* LCOV_EXCL_STOP */ namespace Implementation { From ac3ad3e76c9c5c66dc8be4d6dfc8b120e0446528 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Vladim=C3=ADr=20Vondru=C5=A1?= Date: Wed, 1 Jun 2022 10:38:04 +0200 Subject: [PATCH 2/3] TestSuite: don't go through the container twice in Compare::Container. Since this is a failure case, it doesn't really matter if it takes 10x more time at runtime. But it definitely adds more work for the compiler, and that's where most of human time is usually spent -- iterating on the test code. So don't. --- src/Corrade/TestSuite/Compare/Container.h | 46 ++++++++++++----------- 1 file changed, 25 insertions(+), 21 deletions(-) diff --git a/src/Corrade/TestSuite/Compare/Container.h b/src/Corrade/TestSuite/Compare/Container.h index 31c2d07e5..09c63fc87 100644 --- a/src/Corrade/TestSuite/Compare/Container.h +++ b/src/Corrade/TestSuite/Compare/Container.h @@ -69,22 +69,33 @@ template class Comparator> { private: const T* _actualContents; const T* _expectedContents; + std::size_t _firstDifferent; }; template ComparisonStatusFlags Comparator>::operator()(const T& actual, const T& expected) { _actualContents = &actual; _expectedContents = &expected; + ComparisonStatusFlags status; if(_actualContents->size() != _expectedContents->size()) - return ComparisonStatusFlag::Failed; + status = ComparisonStatusFlag::Failed; - /* Recursively use comparator on the values */ + /* Recursively use the comparator on the values, find the first different + item in the common prefix. If there's none, then the first different + item is right after the common prefix, and if both have the same size + then it means the containers are the same. */ Comparator::type> comparator; - for(std::size_t i = 0; i != _actualContents->size(); ++i) - if(comparator((*_actualContents)[i], (*_expectedContents)[i]) & ComparisonStatusFlag::Failed) - return ComparisonStatusFlag::Failed; + const std::size_t commonPrefixSize = Utility::min(_actualContents->size(), _expectedContents->size()); + _firstDifferent = commonPrefixSize; + for(std::size_t i = 0; i != commonPrefixSize; ++i) { + if(comparator((*_actualContents)[i], (*_expectedContents)[i]) & ComparisonStatusFlag::Failed) { + _firstDifferent = i; + status = ComparisonStatusFlag::Failed; + break; + } + } - return {}; + return status; } template void Comparator>::printMessage(ComparisonStatusFlags, Utility::Debug& out, const char* actual, const char* expected) const { @@ -96,21 +107,14 @@ template void Comparator>::printMessage(Compariso out << *_actualContents << Utility::Debug::newline << " but expected\n " << *_expectedContents << Utility::Debug::newline << " "; - Comparator::type> comparator; - for(std::size_t i = 0, end = Utility::max(_actualContents->size(), _expectedContents->size()); i != end; ++i) { - if(_actualContents->size() > i && _expectedContents->size() > i && - !(comparator((*_actualContents)[i], (*_expectedContents)[i]) & ComparisonStatusFlag::Failed)) continue; - - if(_actualContents->size() <= i) - out << "Expected has" << (*_expectedContents)[i]; - else if(_expectedContents->size() <= i) - out << "Actual has" << (*_actualContents)[i]; - else - out << "Actual" << (*_actualContents)[i] << "but" << (*_expectedContents)[i] << "expected"; - - out << "on position" << i << Utility::Debug::nospace << "."; - break; - } + if(_actualContents->size() <= _firstDifferent) + out << "Expected has" << (*_expectedContents)[_firstDifferent]; + else if(_expectedContents->size() <= _firstDifferent) + out << "Actual has" << (*_actualContents)[_firstDifferent]; + else + out << "Actual" << (*_actualContents)[_firstDifferent] << "but" << (*_expectedContents)[_firstDifferent] << "expected"; + + out << "on position" << _firstDifferent << Utility::Debug::nospace << "."; } #endif From 468afdbd05db3b80da279f065b7f70b64ffad844 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Vladim=C3=ADr=20Vondru=C5=A1?= Date: Wed, 1 Jun 2022 11:01:55 +0200 Subject: [PATCH 3/3] TestSuite: make a non-templated base for Compare::Container. Same as was done for the generic Comparator, but here it's quite a lot heavier so the impact should be bigger. --- src/Corrade/TestSuite/CMakeLists.txt | 1 + src/Corrade/TestSuite/Comparator.cpp | 2 +- src/Corrade/TestSuite/Comparator.h | 12 ++-- src/Corrade/TestSuite/Compare/Container.cpp | 62 +++++++++++++++++++++ src/Corrade/TestSuite/Compare/Container.h | 57 ++++++++++--------- 5 files changed, 99 insertions(+), 35 deletions(-) create mode 100644 src/Corrade/TestSuite/Compare/Container.cpp diff --git a/src/Corrade/TestSuite/CMakeLists.txt b/src/Corrade/TestSuite/CMakeLists.txt index 1a3f3be8d..55ec26064 100644 --- a/src/Corrade/TestSuite/CMakeLists.txt +++ b/src/Corrade/TestSuite/CMakeLists.txt @@ -32,6 +32,7 @@ set(CorradeTestSuite_SRCS Comparator.cpp Tester.cpp + Compare/Container.cpp Compare/File.cpp Compare/FileToString.cpp Compare/FloatingPoint.cpp diff --git a/src/Corrade/TestSuite/Comparator.cpp b/src/Corrade/TestSuite/Comparator.cpp index 7475c7b9b..d55870d1c 100644 --- a/src/Corrade/TestSuite/Comparator.cpp +++ b/src/Corrade/TestSuite/Comparator.cpp @@ -60,7 +60,7 @@ Utility::Debug& operator<<(Utility::Debug& debug, const ComparisonStatusFlags va namespace Implementation { -void ComparatorBase::printMessage(ComparisonStatusFlags, Utility::Debug& out, const char* const actual, const char* const expected, void(*printer)(Utility::Debug&, const void*)) { +void ComparatorBase::printMessage(ComparisonStatusFlags, Utility::Debug& out, const char* const actual, const char* const expected, void(*printer)(Utility::Debug&, const void*)) const { CORRADE_INTERNAL_ASSERT(actualValue && expectedValue); out << "Values" << actual << "and" << expected << "are not the same, actual is\n "; printer(out, actualValue); diff --git a/src/Corrade/TestSuite/Comparator.h b/src/Corrade/TestSuite/Comparator.h index cda63f217..8a8b7b691 100644 --- a/src/Corrade/TestSuite/Comparator.h +++ b/src/Corrade/TestSuite/Comparator.h @@ -114,15 +114,13 @@ namespace Implementation { class CORRADE_TESTSUITE_EXPORT ComparatorBase { public: - explicit ComparatorBase(): actualValue{}, expectedValue{} {} - - void saveDiagnostic(ComparisonStatusFlags status, Utility::Debug& out, Containers::StringView path); + void saveDiagnostic(ComparisonStatusFlags status, Utility::Debug& out, Containers::StringView path); // TODO why even this?? protected: - void printMessage(ComparisonStatusFlags status, Utility::Debug& out, const char* actual, const char* expected, void(*printer)(Utility::Debug&, const void*)); + void printMessage(ComparisonStatusFlags status, Utility::Debug& out, const char* actual, const char* expected, void(*printer)(Utility::Debug&, const void*)) const; - const void* actualValue; - const void* expectedValue; + const void* actualValue{}; + const void* expectedValue{}; }; } @@ -276,7 +274,7 @@ template ComparisonStatusFlags Comparator::operator()(const T& actua return ComparisonStatusFlag::Failed; } -template void Comparator::printMessage(ComparisonStatusFlags status, Utility::Debug& out, const char* actual, const char* expected) { +template void Comparator::printMessage(const ComparisonStatusFlags status, Utility::Debug& out, const char* const actual, const char* const expected) { Implementation::ComparatorBase::printMessage(status, out, actual, expected, [](Utility::Debug& out, const void* value) { out << *static_cast(value); diff --git a/src/Corrade/TestSuite/Compare/Container.cpp b/src/Corrade/TestSuite/Compare/Container.cpp new file mode 100644 index 000000000..856b0bd05 --- /dev/null +++ b/src/Corrade/TestSuite/Compare/Container.cpp @@ -0,0 +1,62 @@ +/* + This file is part of Corrade. + + Copyright © 2007, 2008, 2009, 2010, 2011, 2012, 2013, 2014, 2015, 2016, + 2017, 2018, 2019, 2020, 2021, 2022 + Vladimír Vondruš + + Permission is hereby granted, free of charge, to any person obtaining a + copy of this software and associated documentation files (the "Software"), + to deal in the Software without restriction, including without limitation + the rights to use, copy, modify, merge, publish, distribute, sublicense, + and/or sell copies of the Software, and to permit persons to whom the + Software is furnished to do so, subject to the following conditions: + + The above copyright notice and this permission notice shall be included + in all copies or substantial portions of the Software. + + THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR + IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, + FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL + THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER + LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING + FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER + DEALINGS IN THE SOFTWARE. +*/ + +#include "Container.h" + +namespace Corrade { namespace TestSuite { namespace Implementation { + +void ContainerComparatorBase::printMessage(ComparisonStatusFlags, Utility::Debug& out, const char* const actual, const char* const expected, void(*printer)(Utility::Debug&, const void*), void(*itemPrinter)(Utility::Debug&, const void*, std::size_t)) const { + CORRADE_INTERNAL_ASSERT(_actualContents && _expectedContents); + + out << "Containers" << actual << "and" << expected << "have different"; + if(_actualContentsSize != _expectedContentsSize) + out << "size, actual" << _actualContentsSize << "but" << _expectedContentsSize << "expected. Actual contents:\n "; + else + out << "contents, actual:\n "; + + printer(out, _actualContents); + out << Utility::Debug::newline << " but expected\n "; + printer(out, _expectedContents); + out << Utility::Debug::newline << " "; + + if(_actualContentsSize <= _firstDifferent) { + out << "Expected has"; + itemPrinter(out, _expectedContents, _firstDifferent); + } else if(_expectedContentsSize <= _firstDifferent) { + out << "Actual has"; + itemPrinter(out, _actualContents, _firstDifferent); + } else { + out << "Actual"; + itemPrinter(out, _actualContents, _firstDifferent); + out << "but"; + itemPrinter(out, _expectedContents, _firstDifferent); + out << "expected"; + } + + out << "on position" << _firstDifferent << Utility::Debug::nospace << "."; +} + +}}} diff --git a/src/Corrade/TestSuite/Compare/Container.h b/src/Corrade/TestSuite/Compare/Container.h index 09c63fc87..f5993fff8 100644 --- a/src/Corrade/TestSuite/Compare/Container.h +++ b/src/Corrade/TestSuite/Compare/Container.h @@ -59,36 +59,48 @@ template class Container {}; } +namespace Implementation { + +class CORRADE_TESTSUITE_EXPORT ContainerComparatorBase { + protected: + void printMessage(ComparisonStatusFlags status, Utility::Debug& out, const char* actual, const char* expected, void(*printer)(Utility::Debug&, const void*), void(*itemPrinter)(Utility::Debug&, const void*, std::size_t)) const; + + const void* _actualContents{}; + const void* _expectedContents{}; + std::size_t _actualContentsSize{}; + std::size_t _expectedContentsSize{}; + std::size_t _firstDifferent{}; +}; + +} + #ifndef DOXYGEN_GENERATING_OUTPUT -template class Comparator> { +template class Comparator>: public Implementation::ContainerComparatorBase { public: ComparisonStatusFlags operator()(const T& actual, const T& expected); void printMessage(ComparisonStatusFlags, Utility::Debug& out, const char* actual, const char* expected) const; - - private: - const T* _actualContents; - const T* _expectedContents; - std::size_t _firstDifferent; }; template ComparisonStatusFlags Comparator>::operator()(const T& actual, const T& expected) { _actualContents = &actual; _expectedContents = &expected; + _actualContentsSize = actual.size(); + _expectedContentsSize = expected.size(); ComparisonStatusFlags status; - if(_actualContents->size() != _expectedContents->size()) + if(_actualContentsSize != _expectedContentsSize) status = ComparisonStatusFlag::Failed; /* Recursively use the comparator on the values, find the first different item in the common prefix. If there's none, then the first different item is right after the common prefix, and if both have the same size then it means the containers are the same. */ - Comparator::type> comparator; - const std::size_t commonPrefixSize = Utility::min(_actualContents->size(), _expectedContents->size()); + Comparator::type> comparator; + const std::size_t commonPrefixSize = Utility::min(_actualContentsSize, _expectedContentsSize); _firstDifferent = commonPrefixSize; for(std::size_t i = 0; i != commonPrefixSize; ++i) { - if(comparator((*_actualContents)[i], (*_expectedContents)[i]) & ComparisonStatusFlag::Failed) { + if(comparator(actual[i], expected[i]) & ComparisonStatusFlag::Failed) { _firstDifferent = i; status = ComparisonStatusFlag::Failed; break; @@ -98,23 +110,14 @@ template ComparisonStatusFlags Comparator>::opera return status; } -template void Comparator>::printMessage(ComparisonStatusFlags, Utility::Debug& out, const char* actual, const char* expected) const { - out << "Containers" << actual << "and" << expected << "have different"; - if(_actualContents->size() != _expectedContents->size()) - out << "size, actual" << _actualContents->size() << "but" << _expectedContents->size() << "expected. Actual contents:\n "; - else - out << "contents, actual:\n "; - - out << *_actualContents << Utility::Debug::newline << " but expected\n " << *_expectedContents << Utility::Debug::newline << " "; - - if(_actualContents->size() <= _firstDifferent) - out << "Expected has" << (*_expectedContents)[_firstDifferent]; - else if(_expectedContents->size() <= _firstDifferent) - out << "Actual has" << (*_actualContents)[_firstDifferent]; - else - out << "Actual" << (*_actualContents)[_firstDifferent] << "but" << (*_expectedContents)[_firstDifferent] << "expected"; - - out << "on position" << _firstDifferent << Utility::Debug::nospace << "."; +template void Comparator>::printMessage(const ComparisonStatusFlags status, Utility::Debug& out, const char* actual, const char* expected) const { + Implementation::ContainerComparatorBase::printMessage(status, out, actual, expected, + [](Utility::Debug& out, const void* contents) { + out << *static_cast(contents); + }, + [](Utility::Debug& out, const void* contents, std::size_t i) { + out << (*static_cast(contents))[i]; + }); } #endif