From d385f6ed99170d0c50c9f8e0543d4abee648c03f Mon Sep 17 00:00:00 2001 From: amzn-phist <52085794+amzn-phist@users.noreply.github.com> Date: Fri, 23 Jul 2021 11:29:47 -0500 Subject: [PATCH] Addresses feedback from PR review Change DeepCopyNode utility to return a unique_ptr, fix up some string/path usages to avoid temporaries, etc. Signed-off-by: amzn-phist <52085794+amzn-phist@users.noreply.github.com> --- .../Code/Source/Editor/AudioWwiseLoader.cpp | 4 +--- .../Code/Include/Editor/ACETypes.h | 2 +- .../Code/Include/Engine/AudioFileUtils.h | 2 +- .../Code/Source/Editor/AudioControl.cpp | 2 +- .../Code/Source/Editor/AudioControl.h | 12 +++++----- .../Source/Editor/AudioControlsWriter.cpp | 22 +++++++------------ 6 files changed, 18 insertions(+), 26 deletions(-) diff --git a/Gems/AudioEngineWwise/Code/Source/Editor/AudioWwiseLoader.cpp b/Gems/AudioEngineWwise/Code/Source/Editor/AudioWwiseLoader.cpp index 1a3ff121f8..616a30994c 100644 --- a/Gems/AudioEngineWwise/Code/Source/Editor/AudioWwiseLoader.cpp +++ b/Gems/AudioEngineWwise/Code/Source/Editor/AudioWwiseLoader.cpp @@ -9,8 +9,6 @@ #include -#include - #include #include #include @@ -77,7 +75,7 @@ namespace AudioControls isLocalizedLoaded = true; } } - else if (fileName.Extension() == Audio::Wwise::BankExtension && !AZ::StringFunc::Equal(fileName.Native(), Audio::Wwise::InitBank)) + else if (fileName.Extension() == Audio::Wwise::BankExtension && fileName != Audio::Wwise::InitBank) { m_audioSystemImpl->CreateControl( SControlDef(AZStd::string{ fileName.Native() }, eWCT_WWISE_SOUND_BANK, isLocalized, nullptr, subPath)); diff --git a/Gems/AudioSystem/Code/Include/Editor/ACETypes.h b/Gems/AudioSystem/Code/Include/Editor/ACETypes.h index 0bfaa6640c..1ff6112056 100644 --- a/Gems/AudioSystem/Code/Include/Editor/ACETypes.h +++ b/Gems/AudioSystem/Code/Include/Editor/ACETypes.h @@ -41,6 +41,6 @@ namespace AudioControls using FilepathSet = AZStd::set; using XmlAllocator = AZ::rapidxml::memory_pool<>; - inline static XmlAllocator s_xmlAllocator; + inline XmlAllocator s_xmlAllocator; } // namespace AudioControls diff --git a/Gems/AudioSystem/Code/Include/Engine/AudioFileUtils.h b/Gems/AudioSystem/Code/Include/Engine/AudioFileUtils.h index 37952eaa1a..65122662b1 100644 --- a/Gems/AudioSystem/Code/Include/Engine/AudioFileUtils.h +++ b/Gems/AudioSystem/Code/Include/Engine/AudioFileUtils.h @@ -26,7 +26,7 @@ namespace Audio AZStd::vector foundFiles; AZ::IO::FileIOBase::FindFilesCallbackType findFilesCallback = [&foundFiles](const char* file) -> bool { - foundFiles.emplace_back(AZ::IO::FixedMaxPath{ file }.LexicallyNormal()); + foundFiles.emplace_back(AZ::IO::PathView{ file }.LexicallyNormal()); return true; }; diff --git a/Gems/AudioSystem/Code/Source/Editor/AudioControl.cpp b/Gems/AudioSystem/Code/Source/Editor/AudioControl.cpp index c0a8fb8dde..ce268022a8 100644 --- a/Gems/AudioSystem/Code/Source/Editor/AudioControl.cpp +++ b/Gems/AudioSystem/Code/Source/Editor/AudioControl.cpp @@ -345,7 +345,7 @@ namespace AudioControls { for (auto& connectionNode : m_connectionNodes) { - if (TConnectionPtr connection = audioSystemImpl->CreateConnectionFromXMLNode(connectionNode.m_xmlNode, m_type)) + if (TConnectionPtr connection = audioSystemImpl->CreateConnectionFromXMLNode(connectionNode.m_xmlNode.get(), m_type)) { AddConnection(connection); connectionNode.m_isValid = true; diff --git a/Gems/AudioSystem/Code/Source/Editor/AudioControl.h b/Gems/AudioSystem/Code/Source/Editor/AudioControl.h index 494b0465bd..c53f4aa831 100644 --- a/Gems/AudioSystem/Code/Source/Editor/AudioControl.h +++ b/Gems/AudioSystem/Code/Source/Editor/AudioControl.h @@ -25,11 +25,11 @@ namespace AudioControls { SRawConnectionData(AZ::rapidxml::xml_node* node, bool isValid) { - m_xmlNode = DeepCopyNode(node); + m_xmlNode = AZStd::move(DeepCopyNode(node)); m_isValid = isValid; } - AZ::rapidxml::xml_node* m_xmlNode{ nullptr }; + AZStd::unique_ptr> m_xmlNode{}; // indicates if the connection is valid for the currently loaded middleware bool m_isValid{ false }; @@ -40,20 +40,20 @@ namespace AudioControls // will be pointing into the memory pool of an xml document that has gone out of scope. // This function is a rewritten version of 'clone_node' that does the deep copy of strings // into the new destination tree. - static AZ::rapidxml::xml_node* DeepCopyNode(AZ::rapidxml::xml_node* srcNode) + [[nodiscard]] static AZStd::unique_ptr> DeepCopyNode(AZ::rapidxml::xml_node* srcNode) { - AZ::rapidxml::xml_node* destNode = nullptr; + AZStd::unique_ptr> destNode{}; if (srcNode) { XmlAllocator& xmlAlloc(AudioControls::s_xmlAllocator); - destNode = xmlAlloc.allocate_node(srcNode->type()); + destNode.reset(xmlAlloc.allocate_node(srcNode->type())); destNode->name(xmlAlloc.allocate_string(srcNode->name(), srcNode->name_size()), srcNode->name_size()); destNode->value(xmlAlloc.allocate_string(srcNode->value(), srcNode->value_size()), srcNode->value_size()); for (AZ::rapidxml::xml_node* child = srcNode->first_node(); child != nullptr; child = child->next_sibling()) { - destNode->append_node(DeepCopyNode(child)); + destNode->append_node(DeepCopyNode(child).release()); } for (AZ::rapidxml::xml_attribute* attr = srcNode->first_attribute(); attr != nullptr; attr = attr->next_attribute()) diff --git a/Gems/AudioSystem/Code/Source/Editor/AudioControlsWriter.cpp b/Gems/AudioSystem/Code/Source/Editor/AudioControlsWriter.cpp index 80f05804f1..973da60c90 100644 --- a/Gems/AudioSystem/Code/Source/Editor/AudioControlsWriter.cpp +++ b/Gems/AudioSystem/Code/Source/Editor/AudioControlsWriter.cpp @@ -352,19 +352,13 @@ namespace AudioControls { if (node && control && m_audioSystemImpl) { - TXmlNodeList otherNodes = control->m_connectionNodes; - auto end = AZStd::remove_if( - otherNodes.begin(), otherNodes.end(), - [](const SRawConnectionData& connection) - { - return connection.m_isValid; - } - ); - otherNodes.erase(end, otherNodes.end()); - - for (auto& connectionNode : otherNodes) + for (auto& connectionNode : control->m_connectionNodes) { - node->append_node(SRawConnectionData::DeepCopyNode(connectionNode.m_xmlNode)); + if (!connectionNode.m_isValid) + { + auto nodeCopy = SRawConnectionData::DeepCopyNode(connectionNode.m_xmlNode.get()); + node->append_node(nodeCopy.release()); + } } const size_t size = control->ConnectionCount(); @@ -391,7 +385,7 @@ namespace AudioControls IFileUtil* fileUtil = editor ? editor->GetFileUtil() : nullptr; if (fileUtil) { - fileUtil->CheckoutFile(filepath.data(), nullptr); + fileUtil->CheckoutFile(AZ::IO::FixedMaxPath{ filepath }.c_str(), nullptr); } } @@ -402,7 +396,7 @@ namespace AudioControls IFileUtil* fileUtil = editor ? editor->GetFileUtil() : nullptr; if (fileUtil) { - fileUtil->DeleteFromSourceControl(filepath.data(), nullptr); + fileUtil->DeleteFromSourceControl(AZ::IO::FixedMaxPath{ filepath }.c_str(), nullptr); } }