From dcbd8763afcb574c9d323a7626dc5fc3ef38bb1b Mon Sep 17 00:00:00 2001 From: Jonathan Laroche Date: Thu, 12 Dec 2024 11:03:14 -0500 Subject: [PATCH] Fix for missing throws, wrong param name, comment on YAML, changed related test --- src/OpenColorIO/Config.cpp | 41 +++++++++++++------ src/OpenColorIO/OCIOYaml.cpp | 3 ++ src/OpenColorIO/ParseUtils.cpp | 5 ++- .../apphelpers/mergeconfigs/OCIOMYaml.cpp | 15 +++---- tests/cpu/ParseUtils_tests.cpp | 21 +++------- 5 files changed, 49 insertions(+), 36 deletions(-) diff --git a/src/OpenColorIO/Config.cpp b/src/OpenColorIO/Config.cpp index 2b4db90f7..d270953e6 100644 --- a/src/OpenColorIO/Config.cpp +++ b/src/OpenColorIO/Config.cpp @@ -4005,26 +4005,26 @@ const char * Config::getActiveDisplays() const return getImpl()->m_activeDisplaysStr.c_str(); } -void Config::addActiveDisplay(const char * view) +void Config::addActiveDisplay(const char * display) { - if( !view || !view[0] ) + if( !display || !display[0] ) { throw Exception("Active display could not be added to config, display name has to be a " "non-empty name."); } auto it = std::find(getImpl()->m_activeDisplays.begin(), - getImpl()->m_activeDisplays.end(), view); + getImpl()->m_activeDisplays.end(), display); if( it != getImpl()->m_activeDisplays.end() ) { std::ostringstream os; os << "Active display could not be added to config. An active display named '" - << view << "' already exists."; + << display << "' already exists."; throw Exception(os.str().c_str()); } - getImpl()->m_activeDisplays.push_back(view); + getImpl()->m_activeDisplays.push_back(display); getImpl()->m_displayCache.clear(); @@ -4040,8 +4040,8 @@ void Config::removeActiveDisplay(const char * display) "non-empty name."); } - auto it = std::find(getImpl()->m_activeDisplays.begin(), - getImpl()->m_activeDisplays.end(), display); + auto it = std::find( getImpl()->m_activeDisplays.begin(), + getImpl()->m_activeDisplays.end(), display ); if( it != getImpl()->m_activeDisplays.end() ) { @@ -4056,7 +4056,6 @@ void Config::removeActiveDisplay(const char * display) } getImpl()->m_displayCache.clear(); - AutoMutex lock(getImpl()->m_cacheidMutex); getImpl()->resetCacheIDs(); } @@ -4112,6 +4111,17 @@ void Config::addActiveView(const char * view) "non-empty name."); } + auto it = std::find(getImpl()->m_activeViews.begin(), + getImpl()->m_activeViews.end(), view); + + if( it != getImpl()->m_activeViews.end() ) + { + std::ostringstream os; + os << "Active view could not be added to config. An active view named '" + << view << "' already exists."; + throw Exception(os.str().c_str()); + } + getImpl()->m_activeViews.push_back(view); getImpl()->m_displayCache.clear(); @@ -4123,17 +4133,24 @@ void Config::removeActiveView(const char * view) { if( !view || !view[0] ) { - throw Exception("Active view could not be removed from config, view name has to be a " - "non-empty name."); + throw Exception("Active view could not be removed from config, view name has to be a " + "non-empty name."); } - auto it = std::find(getImpl()->m_activeViews.begin(), - getImpl()->m_activeViews.end(), view); + auto it = std::find( getImpl()->m_activeViews.begin(), + getImpl()->m_activeViews.end(), view ); if(it!=getImpl()->m_activeViews.end()) { getImpl()->m_activeViews.erase(it); } + else + { + std::ostringstream os; + os << "Active view could not be removed from config. An active view named '" + << view << "' could not be found."; + throw Exception(os.str().c_str()); + } getImpl()->m_displayCache.clear(); AutoMutex lock(getImpl()->m_cacheidMutex); diff --git a/src/OpenColorIO/OCIOYaml.cpp b/src/OpenColorIO/OCIOYaml.cpp index 6805b325b..9678b7b37 100644 --- a/src/OpenColorIO/OCIOYaml.cpp +++ b/src/OpenColorIO/OCIOYaml.cpp @@ -4999,6 +4999,7 @@ inline void save(YAML::Emitter & out, const Config & config) active_displays.push_back(config.getActiveDisplay(i)); } + // The YAML library will wrap names that use a comma in quotes. out << YAML::Value << YAML::Flow << active_displays; @@ -5010,6 +5011,8 @@ inline void save(YAML::Emitter & out, const Config & config) { active_views.push_back(config.getActiveView(i)); } + + // The YAML library will wrap names that use a comma in quotes. out << YAML::Value << YAML::Flow << active_views; const std::string inactiveCSs = config.getInactiveColorSpaces(); diff --git a/src/OpenColorIO/ParseUtils.cpp b/src/OpenColorIO/ParseUtils.cpp index 64d459a8f..61875f76a 100644 --- a/src/OpenColorIO/ParseUtils.cpp +++ b/src/OpenColorIO/ParseUtils.cpp @@ -715,8 +715,9 @@ static int findEndOfName(const std::string & s, size_t start) if(nameEndPos == (int)std::string::npos) { // We reached the end of the list instead - nameEndPos = s.size(); - isEndFound = true; + std::ostringstream os; + os << "The string '" << s << "' is not correctly formatted. It is missing a closing quote."; + throw Exception(os.str().c_str()); } else { diff --git a/src/OpenColorIO/apphelpers/mergeconfigs/OCIOMYaml.cpp b/src/OpenColorIO/apphelpers/mergeconfigs/OCIOMYaml.cpp index 3b6517054..25f6dcd08 100644 --- a/src/OpenColorIO/apphelpers/mergeconfigs/OCIOMYaml.cpp +++ b/src/OpenColorIO/apphelpers/mergeconfigs/OCIOMYaml.cpp @@ -548,14 +548,14 @@ inline void save(YAML::Emitter & out, const ConfigMerger & merger) out << YAML::Key << "active_displays"; StringUtils::StringVec active_displays; - int nDisplays = p->getNumActiveDisplays(); - active_displays.reserve( nDisplays ); - for (int i = 0; i < nDisplays; i++) - { - active_displays.push_back(p->getActiveDisplay(i)); - } - + int nDisplays = p->getNumActiveDisplays(); + active_displays.reserve( nDisplays ); + for (int i = 0; i < nDisplays; i++) + { + active_displays.push_back(p->getActiveDisplay(i)); + } + // The YAML library will wrap names that use a comma in quotes. out << YAML::Value << YAML::Flow << active_displays; out << YAML::Newline; @@ -568,6 +568,7 @@ inline void save(YAML::Emitter & out, const ConfigMerger & merger) active_views.push_back(p->getActiveView(i)); } + // The YAML library will wrap names that use a comma in quotes. out << YAML::Value << YAML::Flow << active_views; out << YAML::Key << "inactive_colorspaces"; diff --git a/tests/cpu/ParseUtils_tests.cpp b/tests/cpu/ParseUtils_tests.cpp index b46e147d6..dfe53792c 100644 --- a/tests/cpu/ParseUtils_tests.cpp +++ b/tests/cpu/ParseUtils_tests.cpp @@ -436,10 +436,9 @@ OCIO_ADD_TEST(ParseUtils, split_string_env_style) OCIO_CHECK_EQUAL("a", outputvec[1]); OCIO_CHECK_EQUAL("test", outputvec[2]); - outputvec = OCIO::SplitStringEnvStyle(" This : is \": a: test "); - OCIO_CHECK_EQUAL(2, outputvec.size()); - OCIO_CHECK_EQUAL("This", outputvec[0]); - OCIO_CHECK_EQUAL("is \": a: test", outputvec[1]); + OCIO_CHECK_THROW_WHAT( OCIO::SplitStringEnvStyle(" This : is \": a: test "), + OCIO::Exception, + "The string 'This : is \": a: test' is not correctly formatted. It is missing a closing quote."); outputvec = OCIO::SplitStringEnvStyle(" This : is \": a: test \""); OCIO_CHECK_EQUAL(2, outputvec.size()); @@ -464,22 +463,14 @@ OCIO_ADD_TEST(ParseUtils, split_string_env_style) OCIO_CHECK_EQUAL("a", outputvec[1]); OCIO_CHECK_EQUAL("test", outputvec[2]); - outputvec = OCIO::SplitStringEnvStyle(" This : is \": a: test "); - OCIO_CHECK_EQUAL(2, outputvec.size()); - OCIO_CHECK_EQUAL("This", outputvec[0]); - OCIO_CHECK_EQUAL("is \": a: test", outputvec[1]); - outputvec = OCIO::SplitStringEnvStyle(" This : is \": a: test \""); OCIO_CHECK_EQUAL(2, outputvec.size()); OCIO_CHECK_EQUAL("This", outputvec[0]); OCIO_CHECK_EQUAL("is \": a: test \"" , outputvec[1]); - outputvec = OCIO::SplitStringEnvStyle(" This : is : a: test \""); - OCIO_CHECK_EQUAL(4, outputvec.size()); - OCIO_CHECK_EQUAL("This", outputvec[0]); - OCIO_CHECK_EQUAL("is", outputvec[1]); - OCIO_CHECK_EQUAL("a", outputvec[2]); - OCIO_CHECK_EQUAL("test \"", outputvec[3]); + OCIO_CHECK_THROW_WHAT( OCIO::SplitStringEnvStyle(" This : is : a: test \""), + OCIO::Exception, + "The string 'This : is : a: test \"' is not correctly formatted. It is missing a closing quote."); } OCIO_ADD_TEST(ParseUtils, join_string_env_style)