From c36ea36a43d23ec6c5fc63c5da7857fe9d619adc Mon Sep 17 00:00:00 2001 From: GPayne Date: Thu, 7 Oct 2021 00:26:29 -0600 Subject: [PATCH] Code review changes --- include/openspace/scene/scene.h | 15 ++-- src/scene/scene.cpp | 145 +++++++++++++++----------------- src/scene/scene_lua.inl | 13 +-- 3 files changed, 81 insertions(+), 92 deletions(-) diff --git a/include/openspace/scene/scene.h b/include/openspace/scene/scene.h index 979ba119c4..03e7ec71ce 100644 --- a/include/openspace/scene/scene.h +++ b/include/openspace/scene/scene.h @@ -54,7 +54,7 @@ enum class PropertyValueType { Table, Nil }; -typedef std::variant ProfilePropertyLua; +using ProfilePropertyLua = std::variant; class SceneInitializer; @@ -263,7 +263,7 @@ private: * \param L the lua state to push value to * \param value string representation of the value with which to set property */ - void propertyPushProfileValueToLua(ghoul::lua::LuaState& L, std::string& value); + void propertyPushProfileValueToLua(ghoul::lua::LuaState& L, const std::string& value); /** * Accepts string version of a property value from a profile, and processes it @@ -276,7 +276,8 @@ private: * has already been pushed to the lua stack * \return The ProfilePropertyLua variant type translated from string representation */ - ProfilePropertyLua propertyProcessValue(ghoul::lua::LuaState& L, std::string& value); + ProfilePropertyLua propertyProcessValue(ghoul::lua::LuaState& L, + const std::string& value); /** * Accepts string version of a property value from a profile, and returns the @@ -285,7 +286,7 @@ private: * * \param value string representation of the value with which to set property */ - PropertyValueType getPropertyValueType(std::string& value); + PropertyValueType propertyValueType(const std::string& value); /** * Accepts string version of a property value from a profile, and adds it to a vector @@ -296,8 +297,8 @@ private: * \param table the std::vector container which has elements of type T for a lua table */ template - void processPropertyValueTableEntries(ghoul::lua::LuaState& L, std::string& value, - std::vector& table); + void processPropertyValueTableEntries(ghoul::lua::LuaState& L, + const std::string& value, std::vector& table); /** * Handles a lua table entry, creating a vector of the correct variable type based @@ -306,7 +307,7 @@ private: * \param L the lua state to (eventually) push to * \param value string representation of the value with which to set property */ - void handlePropertyLuaTableEntry(ghoul::lua::LuaState& L, std::string& value); + void handlePropertyLuaTableEntry(ghoul::lua::LuaState& L, const std::string& value); /** * Update dependencies. diff --git a/src/scene/scene.cpp b/src/scene/scene.cpp index d13ecab35d..b8b570193d 100644 --- a/src/scene/scene.cpp +++ b/src/scene/scene.cpp @@ -638,114 +638,107 @@ void Scene::setPropertiesFromProfile(const Profile& p) { } } -void Scene::propertyPushProfileValueToLua(ghoul::lua::LuaState& L, std::string& value) +void Scene::propertyPushProfileValueToLua(ghoul::lua::LuaState& L, + const std::string& value) { _valueIsTable = false; ProfilePropertyLua elem = propertyProcessValue(L, value); if (!_valueIsTable) { std::visit(overloaded{ - [&L](const bool& value) { + [&L](const bool value) { ghoul::lua::push(L, value); }, - [&L](const float& value) { + [&L](const float value) { ghoul::lua::push(L, value); }, - [&L](const std::string& value) { + [&L](const std::string value) { ghoul::lua::push(L, value); }, - [&L](const ghoul::lua::nil_t& nilValue) { + [&L](const ghoul::lua::nil_t nilValue) { ghoul::lua::push(L, nilValue); } }, elem); } } -ProfilePropertyLua Scene::propertyProcessValue(ghoul::lua::LuaState& L,std::string& value) +ProfilePropertyLua Scene::propertyProcessValue(ghoul::lua::LuaState& L, + const std::string& value) { ProfilePropertyLua result; - PropertyValueType pType = getPropertyValueType(value); + PropertyValueType pType = propertyValueType(value); switch (pType) { - case PropertyValueType::Boolean: - result = (value == "true") ? true : false; - break; - - case PropertyValueType::Float: - result = std::stof(value); - break; - - case PropertyValueType::Nil: - ghoul::lua::nil_t n; - result = n; - break; - - case PropertyValueType::Table: - trimSurroundingCharacters(value, '{'); - trimSurroundingCharacters(value, '}'); - handlePropertyLuaTableEntry(L, value); - _valueIsTable = true; - break; - - case PropertyValueType::String: - default: - trimSurroundingCharacters(value, '\"'); - trimSurroundingCharacters(value, '['); - trimSurroundingCharacters(value, ']'); - result = value; - break; + case PropertyValueType::Boolean: + result = (value == "true") ? true : false; + break; + case PropertyValueType::Float: + result = std::stof(value); + break; + case PropertyValueType::Nil: + result = ghoul::lua::nil_t(); + break; + case PropertyValueType::Table: + trimSurroundingCharacters(const_cast(value), '{'); + trimSurroundingCharacters(const_cast(value), '}'); + handlePropertyLuaTableEntry(L, value); + _valueIsTable = true; + break; + case PropertyValueType::String: + default: + trimSurroundingCharacters(const_cast(value), '\"'); + trimSurroundingCharacters(const_cast(value), '['); + trimSurroundingCharacters(const_cast(value), ']'); + result = value; + break; } return result; } -void Scene::handlePropertyLuaTableEntry(ghoul::lua::LuaState& L, std::string& value) { - std::string firstValue; - size_t commaPos = 0; - commaPos = value.find(',', commaPos); +void Scene::handlePropertyLuaTableEntry(ghoul::lua::LuaState& L, const std::string& value) +{ + PropertyValueType enclosedType; + size_t commaPos = value.find(',', 0); if (commaPos != std::string::npos) { - firstValue = value.substr(0, commaPos); + enclosedType = propertyValueType(value.substr(0, commaPos)); } else { - firstValue = value; + enclosedType = propertyValueType(value); } - PropertyValueType enclosedType = getPropertyValueType(firstValue); switch (enclosedType) { - case PropertyValueType::Boolean: - LERROR(fmt::format( - "A lua table of bool values is not supported. (processing property {})", - _profilePropertyName) - ); - break; - - case PropertyValueType::Float: - { - std::vector valsF; - processPropertyValueTableEntries(L, value, valsF); - ghoul::lua::push(L, valsF); - } - break; - - case PropertyValueType::String: - { - std::vector valsS; - processPropertyValueTableEntries(L, value, valsS); - ghoul::lua::push(L, valsS); - } - break; - - case PropertyValueType::Table: - default: - LERROR(fmt::format( - "Table-within-a-table values are not supported for profile a " - "property (processing property {})", _profilePropertyName) - ); - break; + case PropertyValueType::Boolean: + LERROR(fmt::format( + "A lua table of bool values is not supported. (processing property {})", + _profilePropertyName) + ); + break; + case PropertyValueType::Float: + { + std::vector valsF; + processPropertyValueTableEntries(L, value, valsF); + ghoul::lua::push(L, valsF); + } + break; + case PropertyValueType::String: + { + std::vector valsS; + processPropertyValueTableEntries(L, value, valsS); + ghoul::lua::push(L, valsS); + } + break; + case PropertyValueType::Table: + default: + LERROR(fmt::format( + "Table-within-a-table values are not supported for profile a " + "property (processing property {})", _profilePropertyName + )); + break; } } template -void Scene::processPropertyValueTableEntries(ghoul::lua::LuaState& L, std::string& value, - std::vector& table) +void Scene::processPropertyValueTableEntries(ghoul::lua::LuaState& L, + const std::string& value, std::vector& table) { size_t commaPos = 0; size_t prevPos = 0; @@ -767,13 +760,13 @@ void Scene::processPropertyValueTableEntries(ghoul::lua::LuaState& L, std::strin catch (std::bad_variant_access& e) { LERROR(fmt::format( "Error attempting to parse profile property setting for " - "{} using value = {}", _profilePropertyName, value) - ); + "{} using value = {}", _profilePropertyName, value + )); } } } -PropertyValueType Scene::getPropertyValueType(std::string& value) { +PropertyValueType Scene::propertyValueType(const std::string& value) { if (luascriptfunctions::isBoolValue(value)) { return PropertyValueType::Boolean; } diff --git a/src/scene/scene_lua.inl b/src/scene/scene_lua.inl index 32a8d9327d..96df5a47a8 100644 --- a/src/scene/scene_lua.inl +++ b/src/scene/scene_lua.inl @@ -904,7 +904,7 @@ int worldRotation(lua_State* L) { * isBoolValue(const std::string& s): * Used to check if a string is a lua bool type. Returns false if not a valid bool string. */ -bool isBoolValue(const std::string& s) { +bool isBoolValue(std::string_view s) { return (s == "true" || s == "false"); } @@ -917,12 +917,7 @@ bool isFloatValue(const std::string& s) { try { float converted = std::numeric_limits::min(); converted = std::stof(s); - if (converted != std::numeric_limits::min()) { - return true; - } - else { - return false; - } + return (converted != std::numeric_limits::min()); } catch (...) { return false; @@ -934,7 +929,7 @@ bool isFloatValue(const std::string& s) { * isNilValue(const std::string& s): * Used to check if a string is a lua 'nil' value. Returns false if not. */ -bool isNilValue(const std::string& s) { +bool isNilValue(std::string_view s) { return (s == "nil"); } @@ -944,7 +939,7 @@ bool isNilValue(const std::string& s) { * Used to check if a string contains a lua table rather than an individual value. * Returns false if not. */ -bool isTableValue(const std::string& s) { +bool isTableValue(std::string_view s) { return ((s.front() == '{') && (s.back() == '}')); }