Skip to content

Update string constants to avoid global constructors - Option 1 #2693

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Update string constants to avoid global constructors - Option 1 #2693

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 12 additions & 4 deletions source/JsMaterialX/JsMaterialXCore/JsElement.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -135,11 +135,19 @@ EMSCRIPTEN_BINDINGS(element)
BIND_ELEMENT_CHILD_FUNC_INSTANCE(VariantAssign, mx::VariantAssign)
BIND_ELEMENT_CHILD_FUNC_INSTANCE(VariantSet, mx::VariantSet)
BIND_ELEMENT_CHILD_FUNC_INSTANCE(Variant, mx::Variant)
.function("setAttribute", &mx::Element::setAttribute)
.function("hasAttribute", &mx::Element::hasAttribute)
.function("getAttribute", &mx::Element::getAttribute)
.function("setAttribute", ems::optional_override([](mx::Element& self, const std::string& attrib, const std::string& value) {
self.setAttribute(attrib, value);
}))
.function("hasAttribute", ems::optional_override([](const mx::Element& self, const std::string& attrib) {
return self.hasAttribute(attrib);
}))
.function("getAttribute", ems::optional_override([](const mx::Element& self, const std::string& attrib) -> const std::string& {
return self.getAttribute(attrib);
}))
.function("getAttributeNames", &mx::Element::getAttributeNames)
.function("removeAttribute", &mx::Element::removeAttribute)
.function("removeAttribute", ems::optional_override([](mx::Element& self, const std::string& attrib) {
self.removeAttribute(attrib);
}))
.function("getSelf", ems::select_overload<mx::ConstElementPtr()const>(&mx::Element::getSelf))
.function("getParent", ems::select_overload<mx::ConstElementPtr()const>(&mx::Element::getParent))
.function("getRoot", ems::select_overload<mx::ConstElementPtr()const>(&mx::Element::getRoot))
Expand Down
7 changes: 4 additions & 3 deletions source/MaterialXCore/Element.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -190,8 +190,9 @@ void Element::removeChild(const string& name)
unregisterChildElement(it->second);
}

void Element::setAttribute(const string& attrib, const string& value)
void Element::setAttribute(std::string_view attrib_sv, const string& value)
{
string attrib = string(attrib_sv);
getDocument()->invalidateCache();

if (!_attributeMap.count(attrib))
Expand All @@ -201,9 +202,9 @@ void Element::setAttribute(const string& attrib, const string& value)
_attributeMap[attrib] = value;
}

void Element::removeAttribute(const string& attrib)
void Element::removeAttribute(std::string_view attrib)
{
StringMap::iterator it = _attributeMap.find(attrib);
StringMap::iterator it = _attributeMap.find(string(attrib));
if (it != _attributeMap.end())
{
getDocument()->invalidateCache();
Expand Down
12 changes: 6 additions & 6 deletions source/MaterialXCore/Element.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -479,19 +479,19 @@ class MX_CORE_API Element : public std::enable_shared_from_this<Element>
/// @{

/// Set the value string of the given attribute.
void setAttribute(const string& attrib, const string& value);
void setAttribute(std::string_view attrib, const string& value);
Copy link
Contributor

@lgritz lgritz Nov 22, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why not string_view value as well?

Copy link
Contributor Author

@ld-kerley ld-kerley Nov 22, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

just trying to make as minimal API change as possible - I think there's a good improvement to move to more string_view based API everywhere - but here (And in the other PR) I'm just trying to explore a strategy to avoid global constructor/destructor, moving the const string to string_view - it seemed to make the most sense to change the API here to accept a string_view for the attribute name.

Copy link
Contributor

@lgritz lgritz Nov 23, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm a huge fan of string_view.

Copy link
Contributor Author

@ld-kerley ld-kerley Nov 24, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I hear ya :)

I'm much more familiar with the Shader Gen modules than I am the Core and the requirements there to maintain compatibility.

I do know that string operations from the core are measurable impacts from a performance perspective while loading and querying the MaterialX document, so I to think it would be good for someone to take a closer look.


/// Return true if the given attribute is present.
bool hasAttribute(const string& attrib) const
bool hasAttribute(std::string_view attrib) const
{
return _attributeMap.count(attrib) != 0;
return _attributeMap.count(string(attrib)) != 0;
}

/// Return the value string of the given attribute. If the given attribute
/// is not present, then an empty string is returned.
const string& getAttribute(const string& attrib) const
const string& getAttribute(std::string_view attrib) const
{
StringMap::const_iterator it = _attributeMap.find(attrib);
StringMap::const_iterator it = _attributeMap.find(string(attrib));
return (it != _attributeMap.end()) ? it->second : EMPTY_STRING;
}

Expand Down Expand Up @@ -528,7 +528,7 @@ class MX_CORE_API Element : public std::enable_shared_from_this<Element>
}

/// Remove the given attribute, if present.
void removeAttribute(const string& attrib);
void removeAttribute(std::string_view attrib);

/// @}
/// @name Self And Ancestor Elements
Expand Down
5 changes: 0 additions & 5 deletions source/MaterialXCore/Property.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,11 +7,6 @@

MATERIALX_NAMESPACE_BEGIN

const string PropertyAssign::PROPERTY_ATTRIBUTE = "property";
const string PropertyAssign::GEOM_ATTRIBUTE = "geom";
const string PropertyAssign::COLLECTION_ATTRIBUTE = "collection";
const string PropertySetAssign::PROPERTY_SET_ATTRIBUTE = "propertyset";

//
// PropertyAssign methods
//
Expand Down
8 changes: 4 additions & 4 deletions source/MaterialXCore/Property.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -141,9 +141,9 @@ class MX_CORE_API PropertyAssign : public ValueElement

public:
static const string CATEGORY;
static const string PROPERTY_ATTRIBUTE;
static const string GEOM_ATTRIBUTE;
static const string COLLECTION_ATTRIBUTE;
static constexpr std::string_view PROPERTY_ATTRIBUTE = "property";
static constexpr std::string_view GEOM_ATTRIBUTE = "geom";
static constexpr std::string_view COLLECTION_ATTRIBUTE = "collection";
};

/// @class PropertySet
Expand Down Expand Up @@ -263,7 +263,7 @@ class MX_CORE_API PropertySetAssign : public GeomElement

public:
static const string CATEGORY;
static const string PROPERTY_SET_ATTRIBUTE;
static constexpr std::string_view PROPERTY_SET_ATTRIBUTE = "propertyset";
};

MATERIALX_NAMESPACE_END
Expand Down