Skip to content

Commit

Permalink
Static analysis optimizations
Browse files Browse the repository at this point in the history 
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.
  • Loading branch information
@jstone-lucasfilm
jstone-lucasfilm committed Jan 15, 2024
1 parent 9edeb38 commit c9a069c
Show file tree
Hide file tree
Showing 8 changed files with 16 additions and 103 deletions.
2 changes: 1 addition & 1 deletion source/MaterialXFormat/XmlIo.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand Down
2 changes: 1 addition & 1 deletion source/MaterialXFormat/XmlIo.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
4 changes: 2 additions & 2 deletions source/MaterialXGenGlsl/GlslResourceBindingContext.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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
Expand Down
6 changes: 3 additions & 3 deletions source/MaterialXGenGlsl/VkResourceBindingContext.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -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
Expand Down Expand Up @@ -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
Expand Down
4 changes: 2 additions & 2 deletions source/MaterialXGenMsl/MslResourceBindingContext.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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
Expand Down
2 changes: 1 addition & 1 deletion source/MaterialXGenOsl/OslShaderGenerator.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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);
}
Expand Down
90 changes: 4 additions & 86 deletions source/MaterialXGraphEditor/Graph.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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
Expand Down Expand 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<mx::TypedElementPtr> elems = mx::findRenderableElements(_graphDoc);
mx::TypedElementPtr typedElem = nullptr;
for (mx::TypedElementPtr elem : elems)
{
mx::TypedElementPtr renderableElem = elem;
mx::NodePtr node = elem->asA<mx::Node>();
if (node == uiNode->getNode())
{
typedElem = elem;
}
}
_renderer->setMaterial(typedElem);
}

void Graph::setRenderMaterial(UiNodePtr node)
{
// For now only surface shaders and materials are considered renderable.
Expand Down Expand Up @@ -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);
Expand Down Expand Up @@ -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;
Expand Down Expand Up @@ -3736,7 +3669,6 @@ void Graph::addNodePopup(bool cursor)
}
}
}
cursor = false;
ImGui::EndPopup();
open_AddPopup = false;
}
Expand Down Expand Up @@ -3778,7 +3710,6 @@ void Graph::searchNodePopup(bool cursor)
}
}
}
cursor = false;
ImGui::EndPopup();
}
}
Expand Down Expand Up @@ -4349,19 +4280,6 @@ int Graph::findNode(int nodeId)
return -1;
}

std::vector<int> Graph::findLinkId(int id)
{
std::vector<int> 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)
Expand Down
9 changes: 2 additions & 7 deletions source/MaterialXGraphEditor/Graph.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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()
Expand Down Expand Up @@ -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<int> findLinkId(int attrId);

// Check if link exists in the current link vector
bool linkExists(Link newLink);

Expand Down Expand Up @@ -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);
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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();
Expand Down

0 comments on commit c9a069c

Please sign in to comment.