From c9a069c1a385ca929732fc1eac2bff33b3723858 Mon Sep 17 00:00:00 2001 From: Jonathan Stone Date: Sat, 13 Jan 2024 19:58:52 -0800 Subject: [PATCH] Static analysis optimizations This changelist addresses a handful of static analysis optimizations flagged by PVS-Studio and cppcheck, including the following: - Pass immutable std::string, FilePath, and FileSearchPath arguments by const reference. - Mark immutable ShaderGenerator references as const. - Prefer std::string::empty over comparison against an empty string. - Remove unused private methods Graph::findLinkId, Graph::findInput, and Graph::selectMaterial. - Remove variable assignments with no impact on code behavior. --- source/MaterialXFormat/XmlIo.cpp | 2 +- source/MaterialXFormat/XmlIo.h | 2 +- .../GlslResourceBindingContext.cpp | 4 +- .../VkResourceBindingContext.cpp | 6 +- .../MslResourceBindingContext.cpp | 4 +- source/MaterialXGenOsl/OslShaderGenerator.cpp | 2 +- source/MaterialXGraphEditor/Graph.cpp | 90 +------------------ source/MaterialXGraphEditor/Graph.h | 9 +- 8 files changed, 16 insertions(+), 103 deletions(-) diff --git a/source/MaterialXFormat/XmlIo.cpp b/source/MaterialXFormat/XmlIo.cpp index 67d41225a4..2bc0908008 100644 --- a/source/MaterialXFormat/XmlIo.cpp +++ b/source/MaterialXFormat/XmlIo.cpp @@ -353,7 +353,7 @@ void readFromXmlFile(DocumentPtr doc, FilePath filename, FileSearchPath searchPa documentFromXml(doc, xmlDoc, searchPath, readOptions); } -void readFromXmlString(DocumentPtr doc, const string& str, FileSearchPath searchPath, const XmlReadOptions* readOptions) +void readFromXmlString(DocumentPtr doc, const string& str, const FileSearchPath& searchPath, const XmlReadOptions* readOptions) { std::istringstream stream(str); readFromXmlStream(doc, stream, searchPath, readOptions); diff --git a/source/MaterialXFormat/XmlIo.h b/source/MaterialXFormat/XmlIo.h index b098c0fb3a..3aa8d41ed5 100644 --- a/source/MaterialXFormat/XmlIo.h +++ b/source/MaterialXFormat/XmlIo.h @@ -146,7 +146,7 @@ MX_FORMAT_API void readFromXmlFile(DocumentPtr doc, /// If provided, then the given options will affect the behavior of the /// read function. Defaults to a null pointer. /// @throws ExceptionParseError if the document cannot be parsed. -MX_FORMAT_API void readFromXmlString(DocumentPtr doc, const string& str, FileSearchPath searchPath = FileSearchPath(), const XmlReadOptions* readOptions = nullptr); +MX_FORMAT_API void readFromXmlString(DocumentPtr doc, const string& str, const FileSearchPath& searchPath = FileSearchPath(), const XmlReadOptions* readOptions = nullptr); /// @} /// @name Write Functions diff --git a/source/MaterialXGenGlsl/GlslResourceBindingContext.cpp b/source/MaterialXGenGlsl/GlslResourceBindingContext.cpp index 3aad355d60..925aa2cd0c 100644 --- a/source/MaterialXGenGlsl/GlslResourceBindingContext.cpp +++ b/source/MaterialXGenGlsl/GlslResourceBindingContext.cpp @@ -58,7 +58,7 @@ void GlslResourceBindingContext::emitDirectives(GenContext& context, ShaderStage void GlslResourceBindingContext::emitResourceBindings(GenContext& context, const VariableBlock& uniforms, ShaderStage& stage) { - ShaderGenerator& generator = context.getShaderGenerator(); + const ShaderGenerator& generator = context.getShaderGenerator(); const Syntax& syntax = generator.getSyntax(); // First, emit all value uniforms in a block with single layout binding @@ -108,7 +108,7 @@ void GlslResourceBindingContext::emitStructuredResourceBindings(GenContext& cont ShaderStage& stage, const std::string& structInstanceName, const std::string& arraySuffix) { - ShaderGenerator& generator = context.getShaderGenerator(); + const ShaderGenerator& generator = context.getShaderGenerator(); const Syntax& syntax = generator.getSyntax(); // Glsl structures need to be aligned. We make a best effort to base align struct members and add diff --git a/source/MaterialXGenGlsl/VkResourceBindingContext.cpp b/source/MaterialXGenGlsl/VkResourceBindingContext.cpp index 38e7186f4a..3c5d8b6bb0 100644 --- a/source/MaterialXGenGlsl/VkResourceBindingContext.cpp +++ b/source/MaterialXGenGlsl/VkResourceBindingContext.cpp @@ -24,7 +24,7 @@ void VkResourceBindingContext::initialize() void VkResourceBindingContext::emitDirectives(GenContext& context, ShaderStage& stage) { - ShaderGenerator& generator = context.getShaderGenerator(); + const ShaderGenerator& generator = context.getShaderGenerator(); // Write shader stage directives for Vulkan compliance std::string shaderStage; @@ -45,7 +45,7 @@ void VkResourceBindingContext::emitDirectives(GenContext& context, ShaderStage& void VkResourceBindingContext::emitResourceBindings(GenContext& context, const VariableBlock& uniforms, ShaderStage& stage) { - ShaderGenerator& generator = context.getShaderGenerator(); + const ShaderGenerator& generator = context.getShaderGenerator(); const Syntax& syntax = generator.getSyntax(); // First, emit all value uniforms in a block with single layout binding @@ -95,7 +95,7 @@ void VkResourceBindingContext::emitStructuredResourceBindings(GenContext& contex ShaderStage& stage, const std::string& structInstanceName, const std::string& arraySuffix) { - ShaderGenerator& generator = context.getShaderGenerator(); + const ShaderGenerator& generator = context.getShaderGenerator(); const Syntax& syntax = generator.getSyntax(); // Glsl structures need to be aligned. We make a best effort to base align struct members and add diff --git a/source/MaterialXGenMsl/MslResourceBindingContext.cpp b/source/MaterialXGenMsl/MslResourceBindingContext.cpp index 7b61f41854..5d6cbab6f9 100644 --- a/source/MaterialXGenMsl/MslResourceBindingContext.cpp +++ b/source/MaterialXGenMsl/MslResourceBindingContext.cpp @@ -32,7 +32,7 @@ void MslResourceBindingContext::emitDirectives(GenContext&, ShaderStage&) void MslResourceBindingContext::emitResourceBindings(GenContext& context, const VariableBlock& uniforms, ShaderStage& stage) { - ShaderGenerator& generator = context.getShaderGenerator(); + const ShaderGenerator& generator = context.getShaderGenerator(); // First, emit all value uniforms in a block with single layout binding bool hasValueUniforms = false; @@ -69,7 +69,7 @@ void MslResourceBindingContext::emitStructuredResourceBindings(GenContext& conte ShaderStage& stage, const std::string& structInstanceName, const std::string& arraySuffix) { - ShaderGenerator& generator = context.getShaderGenerator(); + const ShaderGenerator& generator = context.getShaderGenerator(); const size_t baseAlignment = 16; // Values are adjusted based on diff --git a/source/MaterialXGenOsl/OslShaderGenerator.cpp b/source/MaterialXGenOsl/OslShaderGenerator.cpp index f16770174b..0ef647246a 100644 --- a/source/MaterialXGenOsl/OslShaderGenerator.cpp +++ b/source/MaterialXGenOsl/OslShaderGenerator.cpp @@ -651,7 +651,7 @@ void OslShaderGenerator::emitMetadata(const ShaderPort* port, ShaderStage& stage { emitLineEnd(stage, false); emitScopeBegin(stage, Syntax::DOUBLE_SQUARE_BRACKETS); - for (auto line : metadataLines) + for (const auto& line : metadataLines) { emitLine(line, stage, false); } diff --git a/source/MaterialXGraphEditor/Graph.cpp b/source/MaterialXGraphEditor/Graph.cpp index f0d1791143..181ea9e591 100644 --- a/source/MaterialXGraphEditor/Graph.cpp +++ b/source/MaterialXGraphEditor/Graph.cpp @@ -200,7 +200,7 @@ void Graph::loadStandardLibraries() } } -mx::DocumentPtr Graph::loadDocument(mx::FilePath filename) +mx::DocumentPtr Graph::loadDocument(const mx::FilePath& filename) { mx::FilePathVec libraryFolders = { "libraries" }; _libraryFolders = libraryFolders; @@ -430,14 +430,8 @@ int Graph::findLinkPosition(int id) bool Graph::checkPosition(UiNodePtr node) { - if (node->getMxElement() != nullptr) - { - if (node->getMxElement()->getAttribute("xpos") != "") - { - return true; - } - } - return false; + return node->getMxElement() && + !node->getMxElement()->getAttribute("xpos").empty(); } // Calculate the total vertical space the node level takes up @@ -710,23 +704,6 @@ void Graph::setPinColor() _pinColor.insert(std::make_pair("stringarray", ImColor(120, 180, 100))); } -void Graph::selectMaterial(UiNodePtr uiNode) -{ - // Find renderable element that corresponds with material UiNode - std::vector elems = mx::findRenderableElements(_graphDoc); - mx::TypedElementPtr typedElem = nullptr; - for (mx::TypedElementPtr elem : elems) - { - mx::TypedElementPtr renderableElem = elem; - mx::NodePtr node = elem->asA(); - if (node == uiNode->getNode()) - { - typedElem = elem; - } - } - _renderer->setMaterial(typedElem); -} - void Graph::setRenderMaterial(UiNodePtr node) { // For now only surface shaders and materials are considered renderable. @@ -2028,7 +2005,7 @@ UiPinPtr Graph::getPin(ed::PinId pinId) return nullPin; } -void Graph::drawPinIcon(std::string type, bool connected, int alpha) +void Graph::drawPinIcon(const std::string& type, bool connected, int alpha) { ax::Drawing::IconType iconType = ax::Drawing::IconType::Flow; ImColor color = ImColor(0, 0, 0, 255); @@ -2105,50 +2082,6 @@ bool Graph::readOnly() return _currGraphElem->getActiveSourceUri() != _graphDoc->getActiveSourceUri(); } -mx::InputPtr Graph::findInput(mx::InputPtr nodeInput, const std::string& name) -{ - if (_isNodeGraph) - { - for (UiNodePtr uiNode : _graphNodes) - { - if (uiNode->getNode()) - { - for (mx::InputPtr input : uiNode->getNode()->getActiveInputs()) - { - if (input->getInterfaceInput()) - { - if (input->getInterfaceInput() == nodeInput) - { - return input; - } - } - } - } - } - } - else - { - if (_currUiNode->getNodeGraph()) - { - for (mx::NodePtr node : _currUiNode->getNodeGraph()->getNodes()) - { - for (mx::InputPtr input : node->getActiveInputs()) - { - if (input->getInterfaceInput()) - { - - if (input->getInterfaceName() == name) - { - return input; - } - } - } - } - } - } - return nullptr; -} - void Graph::drawOutputPins(UiNodePtr node, const std::string& longestInputLabel) { std::string longestLabel = longestInputLabel; @@ -3736,7 +3669,6 @@ void Graph::addNodePopup(bool cursor) } } } - cursor = false; ImGui::EndPopup(); open_AddPopup = false; } @@ -3778,7 +3710,6 @@ void Graph::searchNodePopup(bool cursor) } } } - cursor = false; ImGui::EndPopup(); } } @@ -4349,19 +4280,6 @@ int Graph::findNode(int nodeId) return -1; } -std::vector Graph::findLinkId(int id) -{ - std::vector ids; - for (const Link& link : _currLinks) - { - if (link._startAttr == id || link._endAttr == id) - { - ids.push_back(link.id); - } - } - return ids; -} - bool Graph::edgeExists(UiEdge newEdge) { if (_currEdge.size() > 0) diff --git a/source/MaterialXGraphEditor/Graph.h b/source/MaterialXGraphEditor/Graph.h index 821ecf71fd..965ec519b7 100644 --- a/source/MaterialXGraphEditor/Graph.h +++ b/source/MaterialXGraphEditor/Graph.h @@ -66,7 +66,7 @@ class Graph int viewWidth, int viewHeight); - mx::DocumentPtr loadDocument(mx::FilePath filename); + mx::DocumentPtr loadDocument(const mx::FilePath& filename); void drawGraph(ImVec2 mousePos); RenderViewPtr getRenderer() @@ -106,9 +106,6 @@ class Graph // Find link position in current links vector from link id int findLinkPosition(int id); - // Find link from attribute id - std::vector findLinkId(int attrId); - // Check if link exists in the current link vector bool linkExists(Link newLink); @@ -139,7 +136,7 @@ class Graph void setPinColor(); // Based on the pin icon function in the ImGui Node Editor blueprints-example.cpp - void drawPinIcon(std::string type, bool connected, int alpha); + void drawPinIcon(const std::string& type, bool connected, int alpha); UiPinPtr getPin(ed::PinId id); void drawInputPin(UiPinPtr pin); @@ -189,7 +186,6 @@ class Graph // Add input pointer to node based on input pin void addNodeInput(UiNodePtr node, mx::InputPtr& input); - mx::InputPtr findInput(mx::InputPtr input, const std::string& name); void upNodeGraph(); // Set the value of the selected node constants in the node property editor @@ -222,7 +218,6 @@ class Graph void shaderPopup(); void updateMaterials(mx::InputPtr input = nullptr, mx::ValuePtr value = nullptr); - void selectMaterial(UiNodePtr node); // Allow for camera manipulation of render view window void handleRenderViewInputs();