Improve Syntax::makeValidName function#2598
Conversation
…ords. This has a couple of benefits, we don't need to populate each ShaderGraphs identifier list with every reserved word in the syntax for each graph we create. Also there are a couple of places where makeValidName() is called - namely in SourceCodeNode and CompoundNode, to generate the names of functions, which currently do not ensure that reserved words are not generated.
|
This looks like a great change. Feels much better and safer to keep the reserved words check fully contained within the |
| // | ||
|
|
||
| ShaderGraph::ShaderGraph(const ShaderGraph* parent, const string& name, ConstDocumentPtr document, const StringSet& reservedWords) : | ||
| ShaderGraph::ShaderGraph(const ShaderGraph* parent, const string& name, ConstDocumentPtr document, const StringSet& /*reservedWords*/) : |
There was a problem hiding this comment.
Perhaps remove the reservedWords parameter completely since it's not used now?
This constructor is only ever called from within ShaderGraph.cpp, in the ShaderGraph::create methods, and I doubt anyone has ever used it explicitly from an integration. It's not exposed in python. So I think it should be safe to change the signature.
|
Thanks @niklasharrysson - yeah I was debating removing it from the interface too - but wasn't sure how worried I needed to be about backwards compatibility. I think I'll maybe it's better to merge this PR (just need someone to hit the approve button), and then I can put up a cleanup PR afterwards, that way if the clean up causes someone an issue - we can easily revert it. |
jstone-lucasfilm
left a comment
jstone-lucasfilm
left a comment
There was a problem hiding this comment.
This looks fine to me as well, thanks @ld-kerley!
jstone-lucasfilm
changed the title
Syntax::makeValidName function
jstone-lucasfilm
merged commit b323afd
into
AcademySoftwareFoundation:main
3 participants
This PR improves the logic around how valid names are generated with consideration for reserved words defined in specific Syntax objects.
Currently the shader generator calls
Syntax::makeIdentifier()to ensure that variable names are legal. This compares the current name against a map of already used identifiers. The reserved words for a syntax are prepopulated in this map.There are some places, namely here and here, where we generate names that need to be legal, but do not call
makeIdentifier()but instead just callmakeValidName(). Which got me to thinking that makemakeValidName()should be doing the reserved word check.Moving the check to
makeValidName()means we do not have to copy all of the reserved words in to the identifier map for eachShaderGraphobject we create, as well as plugging the potential bug of being able to generate illegal function names.I didn't add a warning - but I wonder if we should emit a warning if a reserved word is used as a variable name?