Skip to content

Commit

Permalink
Review fixes:
Browse files Browse the repository at this point in the history 
-Adding test cases and some parsing fixes
-Fix for join
-Fix for bug in split string and move order of functions
  • Loading branch information
@larochj
larochj committed Dec 12, 2024
1 parent f85eb05 commit 6c41780
Show file tree
Hide file tree
Showing 3 changed files with 179 additions and 45 deletions.
66 changes: 43 additions & 23 deletions src/OpenColorIO/Config.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -3411,7 +3411,7 @@ void Config::removeSharedView(const char * view)
{
std::ostringstream os;
os << "Shared view could not be removed from config. A shared view named '"
<< view << "' could be be found.";
<< view << "' could not be found.";
throw Exception(os.str().c_str());
}
}
Expand Down Expand Up @@ -4005,20 +4005,54 @@ const char * Config::getActiveDisplays() const
return getImpl()->m_activeDisplaysStr.c_str();
}

void Config::addActiveDisplay(const char * view)
{
if( !view || !view[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);

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.";
throw Exception(os.str().c_str());
}

getImpl()->m_activeDisplays.push_back(view);

getImpl()->m_displayCache.clear();

AutoMutex lock(getImpl()->m_cacheidMutex);
getImpl()->resetCacheIDs();
}

void Config::removeActiveDisplay(const char * display)
{
if( !display || !display[0] )
{
throw Exception("Active display could not be removed from config, display name has to be a "
"non-empty name.");
throw Exception("Active display could not be removed from config, display name has to be a "
"non-empty name.");
}

auto it = std::find(getImpl()->m_activeDisplays.begin(),
getImpl()->m_activeDisplays.end(), display);

if(it!=getImpl()->m_activeDisplays.end())
if( it != getImpl()->m_activeDisplays.end() )
{
getImpl()->m_activeDisplays.erase(it);
}
else
{
getImpl()->m_activeDisplays.erase(it);
std::ostringstream os;
os << "Active display could not be removed from config. An active display named '"
<< display << "' could not be found.";
throw Exception(os.str().c_str());
}

getImpl()->m_displayCache.clear();
Expand All @@ -4039,7 +4073,8 @@ void Config::clearActiveDisplays()

const char * Config::getActiveDisplay( int index ) const
{
if( index<0 || index >= static_cast<int>(getImpl()->m_activeDisplays.size()))
if( index<0 ||
index >= static_cast<int>(getImpl()->m_activeDisplays.size()))
{
return nullptr;
}
Expand All @@ -4052,22 +4087,6 @@ int Config::getNumActiveDisplays() const
return static_cast<int>(getImpl()->m_activeDisplays.size());
}

void Config::addActiveDisplay(const char * view)
{
if( !view || !view[0] )
{
throw Exception("Active view could not be added to config, view name has to be a "
"non-empty name.");
}

getImpl()->m_activeDisplays.push_back(view);

getImpl()->m_displayCache.clear();

AutoMutex lock(getImpl()->m_cacheidMutex);
getImpl()->resetCacheIDs();
}

void Config::setActiveViews(const char * views)
{
getImpl()->m_activeViews.clear();
Expand Down Expand Up @@ -4132,7 +4151,8 @@ void Config::clearActiveViews()

const char * Config::getActiveView( int index ) const
{
if( index<0 || index >= static_cast<int>(getImpl()->m_activeViews.size()))
if( index<0 ||
index >= static_cast<int>(getImpl()->m_activeViews.size()))
{
return nullptr;
}
Expand Down
72 changes: 53 additions & 19 deletions src/OpenColorIO/ParseUtils.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -693,29 +693,29 @@ bool StrEqualsCaseIgnore(const std::string & a, const std::string & b)
// Find the end of a name from a list contained in a string.
// The element of the list are separeted by ":" or ",".
// The name can be surrounded by quotes to enable name including theses symbols.
static size_t findEndOfName(const std::string & s, size_t start)
static int findEndOfName(const std::string & s, size_t start)
{
size_t currentPos = start;
size_t nameEndPos = currentPos;
int currentPos = start;
int nameEndPos = currentPos;
bool isEndFound = false;

while( !isEndFound )
{
nameEndPos = s.find_first_of("\",:", currentPos);
if(nameEndPos == std::string::npos)
if(nameEndPos == (int)std::string::npos)
{
// We reached the end of the list
nameEndPos = s.size() - 1;
nameEndPos = s.size();
isEndFound = true;
}
else if( s[nameEndPos] == '"' )
{
// We found a quote, we need to find the next one
nameEndPos = s.find_first_of("\"", nameEndPos + 1);
if(nameEndPos == std::string::npos)
if(nameEndPos == (int)std::string::npos)
{
// We reached the end of the list instead
nameEndPos = s.size() - 1;
nameEndPos = s.size();
isEndFound = true;
}
else
Expand All @@ -729,7 +729,6 @@ static size_t findEndOfName(const std::string & s, size_t start)
s[nameEndPos] == ':' )
{
// We found a symbol separating the elements, we stop here
nameEndPos--;
isEndFound = true;
}
}
Expand All @@ -739,17 +738,28 @@ static size_t findEndOfName(const std::string & s, size_t start)

StringUtils::StringVec SplitStringEnvStyle(const std::string & str)
{
StringUtils::StringVec outputvec;
const std::string s = StringUtils::Trim(str);
size_t currentPos = 0;
if( s.size() == 0 )
{
return {};
}


StringUtils::StringVec outputvec;
int currentPos = 0;
while( s.size() > 0 &&
currentPos < s.size() - 1 )
currentPos <= (int) s.size() )
{
size_t nameEndPos = findEndOfName(s, currentPos);
outputvec.push_back(s.substr(currentPos, nameEndPos - currentPos + 1));
currentPos = nameEndPos + 2;
int nameEndPos = findEndOfName(s, currentPos);
if(nameEndPos > currentPos)
{
outputvec.push_back(s.substr(currentPos, nameEndPos - currentPos));
currentPos = nameEndPos + 1;
}
else
{
outputvec.push_back("");
currentPos += 1;
}
}

for ( auto & val : outputvec )
Expand All @@ -775,15 +785,39 @@ StringUtils::StringVec SplitStringEnvStyle(const std::string & str)
std::string JoinStringEnvStyle(const StringUtils::StringVec & outputvec)
{
std::string result;
for( const auto & val : outputvec )
const int nElement = static_cast<int>(outputvec.size());
if( nElement == 0 )
{
return "";
}

// We check if the value contains a symbol that could be interpreted as a separator
// and if it is not already surrounded by quotes
const std::string& firstValue = outputvec[0];
if( firstValue.find_first_of(",:") != std::string::npos &&
firstValue.size() > 1 &&
firstValue[0] != '"' &&
firstValue[firstValue.size() - 1] != '"' )
{
result += "\"" + firstValue + "\"";
}
else
{
result += firstValue;
}

for( int i = 1; i < nElement; ++i )
{
if( val.find_first_of(',') != std::string::npos )
if( outputvec[i].find_first_of(",:") != std::string::npos &&
outputvec[i].size() > 1 &&
outputvec[i][0] != '"' &&
outputvec[i][outputvec[i].size() - 1] != '"' )
{
result += "\"" + val + "\", ";
result += ", \"" + outputvec[i] + "\"";
}
else
{
result += val + ", ";
result += ", " + outputvec[i];
}
}

Expand Down
86 changes: 83 additions & 3 deletions tests/cpu/ParseUtils_tests.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -395,30 +395,110 @@ OCIO_ADD_TEST(ParseUtils, split_string_env_style)
OCIO_CHECK_EQUAL("a", outputvec[2]);
OCIO_CHECK_EQUAL("test", outputvec[3]);
outputvec.clear();

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]);
outputvec.clear();

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]);
outputvec.clear();

outputvec = OCIO::SplitStringEnvStyle("This:is , a:test ");
OCIO_CHECK_EQUAL(2, outputvec.size());
OCIO_CHECK_EQUAL("This:is", outputvec[0]);
OCIO_CHECK_EQUAL("a:test", outputvec[1]);
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]);
outputvec.clear();

outputvec = OCIO::SplitStringEnvStyle(",,");
OCIO_CHECK_EQUAL(3, outputvec.size());
OCIO_CHECK_EQUAL("", outputvec[0]);
OCIO_CHECK_EQUAL("", outputvec[1]);
OCIO_CHECK_EQUAL("", outputvec[2]);

outputvec = OCIO::SplitStringEnvStyle(" \"This : is \": a: test ");
OCIO_CHECK_EQUAL(3, outputvec.size());
OCIO_CHECK_EQUAL("This : is ", outputvec[0]);
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(3, outputvec.size());
OCIO_CHECK_EQUAL("This : is ", outputvec[0]);
OCIO_CHECK_EQUAL("a", outputvec[1]);
OCIO_CHECK_EQUAL("test", outputvec[2]);

outputvec = OCIO::SplitStringEnvStyle(" \"This , is \": a: test ");
OCIO_CHECK_EQUAL(3, outputvec.size());
OCIO_CHECK_EQUAL("This , is ", outputvec[0]);
OCIO_CHECK_EQUAL("a", outputvec[1]);
OCIO_CHECK_EQUAL("test", outputvec[2]);

outputvec = OCIO::SplitStringEnvStyle(" \"This , is \": a, test ");
OCIO_CHECK_EQUAL(3, outputvec.size());
OCIO_CHECK_EQUAL("This , is ", outputvec[0]);
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_ADD_TEST(ParseUtils, join_string_env_style)
{
StringUtils::StringVec outputvec {"This", "is", "a", "test"};

OCIO_CHECK_EQUAL( "This, is, a, test", OCIO::JoinStringEnvStyle(outputvec) );
outputvec.clear();

outputvec = { "This:is", "a:test" };
OCIO_CHECK_EQUAL( "\"This:is\", \"a:test\"", OCIO::JoinStringEnvStyle(outputvec) );
outputvec.clear();

outputvec = { "", "", "" };
OCIO_CHECK_EQUAL( ", , ", OCIO::JoinStringEnvStyle(outputvec) );
outputvec.clear();

outputvec = { "This : is", "a: test" };
OCIO_CHECK_EQUAL( "\"This : is\", \"a: test\"", OCIO::JoinStringEnvStyle(outputvec) );
outputvec.clear();

outputvec = { "This", "is \": a: test" };
OCIO_CHECK_EQUAL( "This, \"is \": a: test\"", OCIO::JoinStringEnvStyle(outputvec) );
}

OCIO_ADD_TEST(ParseUtils, intersect_string_vecs_case_ignore)
Expand Down

0 comments on commit 6c41780

Please sign in to comment.