From d4a504ff6a9ab1adf4b51fb6a61ab864b27f21bf Mon Sep 17 00:00:00 2001 From: Julien Esseiva Date: Thu, 5 Feb 2026 10:37:03 -0800 Subject: [PATCH 1/2] replace vectors with map/set --- .../orangeinp/detail/DeMorganSimplifier.cc | 84 ++++++++++++------- .../orangeinp/detail/DeMorganSimplifier.hh | 17 +++- 2 files changed, 65 insertions(+), 36 deletions(-) diff --git a/src/orange/orangeinp/detail/DeMorganSimplifier.cc b/src/orange/orangeinp/detail/DeMorganSimplifier.cc index 4f20d01a62..1850cb8c4e 100644 --- a/src/orange/orangeinp/detail/DeMorganSimplifier.cc +++ b/src/orange/orangeinp/detail/DeMorganSimplifier.cc @@ -49,9 +49,28 @@ NodeId DeMorganSimplifier::MatchingNodes::equivalent_node() const DeMorganSimplifier::DeMorganSimplifier(CsgTree const& tree) : tree_(tree), parents_(tree_.size()) { - new_negated_nodes_.resize(tree_.size()); - negated_join_nodes_.resize(tree_.size()); - node_ids_translation_.resize(tree_.size()); +} + +//---------------------------------------------------------------------------// +/*! + * Access or create a translation entry. + */ +DeMorganSimplifier::MatchingNodes& DeMorganSimplifier::translation(NodeId id) +{ + return node_ids_translation_.try_emplace(id).first->second; +} + +//---------------------------------------------------------------------------// +/*! + * Find a translation entry. + */ +DeMorganSimplifier::MatchingNodes const* +DeMorganSimplifier::find_translation(NodeId id) const +{ + auto iter = node_ids_translation_.find(id); + if (iter == node_ids_translation_.end()) + return nullptr; + return &iter->second; } //---------------------------------------------------------------------------// @@ -65,10 +84,16 @@ TransformedTree DeMorganSimplifier::operator()() auto simplified_tree{this->build_simplified_tree()}; std::vector equivalent_nodes; equivalent_nodes.reserve(tree_.size()); - for (auto node_id : range(tree_.size())) + for (auto node_id : range(NodeId{tree_.size()})) { - equivalent_nodes.push_back( - node_ids_translation_[node_id].equivalent_node()); + if (auto const* trans = this->find_translation(node_id)) + { + equivalent_nodes.push_back(trans->equivalent_node()); + } + else + { + equivalent_nodes.push_back(NodeId{}); + } } return {simplified_tree, equivalent_nodes}; } @@ -107,7 +132,7 @@ void DeMorganSimplifier::find_join_negations() tree_[this->dealias(negated->node)])) { // This is a negated join node - negated_join_nodes_[negated->node.get()] = true; + negated_join_nodes_.insert(negated->node); this->add_negation_for_operands(negated->node); } } @@ -148,14 +173,14 @@ void DeMorganSimplifier::add_negation_for_operands(NodeId node_id) { // This negated join node has a join operand, so we'll have to // insert a negated join of that join operand and its operands - negated_join_nodes_[join_operand.get()] = true; + negated_join_nodes_.insert(join_operand); this->add_negation_for_operands(join_operand); } else if (!std::holds_alternative(target_node)) { // Negate each operand unless it's a negated node, in which // case double negation will cancel to the child of that operand - new_negated_nodes_[join_operand.get()] = true; + new_negated_nodes_.insert(join_operand); } } } @@ -192,9 +217,8 @@ CsgTree DeMorganSimplifier::build_simplified_tree() // We're never inserting a negated node pointing to a // joined or negated node so it's child must have an // unmodified equivalent in the simplified tree - CELER_ASSERT(node_ids_translation_[negated->node.get()].unmodified); - negated->node - = node_ids_translation_[negated->node.get()].unmodified; + CELER_ASSERT(this->translation(negated->node).unmodified); + negated->node = this->translation(negated->node).unmodified; } else if (auto* joined = std::get_if(&new_node)) { @@ -205,20 +229,20 @@ CsgTree DeMorganSimplifier::build_simplified_tree() // That means we should find an equivalent node for // each operand, either a simplified negated join or an // unmodified node - CELER_ASSERT(node_ids_translation_[op.get()].equivalent_node()); - op = node_ids_translation_[op.get()].equivalent_node(); + CELER_ASSERT(this->translation(op).equivalent_node()); + op = this->translation(op).equivalent_node(); } } auto [new_id, inserted] = result.insert(std::move(new_node)); - auto& trans = node_ids_translation_[node_id.get()]; + auto& trans = this->translation(node_id); CELER_ASSERT(!trans.unmodified); // Record the new node id for parents of that node trans.unmodified = new_id; // We might have to insert a negated version of that node - if (new_negated_nodes_[node_id.get()]) + if (new_negated_nodes_.count(node_id)) { Node const& target_node{tree_[this->dealias(node_id)]}; CELER_ASSERT(!std::holds_alternative(target_node) @@ -237,9 +261,8 @@ CsgTree DeMorganSimplifier::build_simplified_tree() // new tree. // This is not always the exact same node, e.g., if the volume // points to a negated join, it will still be simplified - CELER_ASSERT(node_ids_translation_[volume.get()].equivalent_node()); - result.insert_volume( - node_ids_translation_[volume.get()].equivalent_node()); + CELER_ASSERT(this->translation(volume).equivalent_node()); + result.insert_volume(this->translation(volume).equivalent_node()); } return result; @@ -274,10 +297,9 @@ bool DeMorganSimplifier::process_negated_joined_nodes(NodeId node_id, { // Redirect parents looking for this node to the new Joined // node which is logically equivalent - CELER_ASSERT( - node_ids_translation_[negated->node.get()].opposite_join); - node_ids_translation_[node_id.get()].simplified_to - = node_ids_translation_[negated->node.get()].opposite_join; + CELER_ASSERT(this->translation(negated->node).opposite_join); + this->translation(node_id).simplified_to + = this->translation(negated->node).opposite_join; return false; } @@ -318,14 +340,13 @@ bool DeMorganSimplifier::process_negated_joined_nodes(NodeId node_id, else if (auto const* joined = std::get_if(target_node)) { // Check if this node needs a simplification - if (negated_join_nodes_[node_id.get()]) + if (negated_join_nodes_.count(node_id)) { // Insert the negated node auto [new_id, inserted] = result.insert(this->build_negated_node(*joined)); // Record that we inserted an opposite join for that node - node_ids_translation_[node_id.get()].opposite_join - = std::move(new_id); + this->translation(node_id).opposite_join = std::move(new_id); } return this->should_insert_join(node_id); } @@ -340,7 +361,7 @@ bool DeMorganSimplifier::process_negated_joined_nodes(NodeId node_id, * * \return Join node with opposite operation and negated operands */ -Joined DeMorganSimplifier::build_negated_node(Joined const& joined) const +Joined DeMorganSimplifier::build_negated_node(Joined const& joined) { // Insert the opposite join auto const& [op, nodes] = joined; @@ -357,9 +378,8 @@ Joined DeMorganSimplifier::build_negated_node(Joined const& joined) const { // We should have recorded that this node was necessary // for a join - CELER_ASSERT(node_ids_translation_[neg->node.get()].unmodified); - operands.push_back( - node_ids_translation_[neg->node.get()].unmodified); + CELER_ASSERT(this->translation(neg->node).unmodified); + operands.push_back(this->translation(neg->node).unmodified); } else { @@ -367,7 +387,7 @@ Joined DeMorganSimplifier::build_negated_node(Joined const& joined) const // version of that operand in the simplified tree. // It's either a simplified join or a negated node operands.push_back([&] { - auto& trans = node_ids_translation_[n.get()]; + auto& trans = this->translation(n); CELER_ASSERT(trans.new_negation || trans.opposite_join); if (trans.new_negation) return trans.new_negation; @@ -406,7 +426,7 @@ bool DeMorganSimplifier::should_insert_join(NodeId node_id) auto has_negated_join_parent = [&](NodeId n) { for (auto p : range(first_node_id_, NodeId{tree_.size()})) { - if (parents_.count({n, p}) && negated_join_nodes_[p.get()]) + if (parents_.count({n, p}) && negated_join_nodes_.count(p)) return true; } return false; diff --git a/src/orange/orangeinp/detail/DeMorganSimplifier.hh b/src/orange/orangeinp/detail/DeMorganSimplifier.hh index 97a9e92af6..18d4017fc9 100644 --- a/src/orange/orangeinp/detail/DeMorganSimplifier.hh +++ b/src/orange/orangeinp/detail/DeMorganSimplifier.hh @@ -6,6 +6,7 @@ //---------------------------------------------------------------------------// #pragma once +#include #include #include #include @@ -64,6 +65,7 @@ class DeMorganSimplifier }; using SparseMatrix2D = std::unordered_set; + using NodeSet = std::unordered_set; //! CsgTree node 0 is always True{} and can't be the parent of any node //! so reuse that bit to tell that a given node is a volume @@ -104,6 +106,8 @@ class DeMorganSimplifier NodeId equivalent_node() const; }; + using NodeMap = std::unordered_map; + // Dereference Aliased nodes NodeId dealias(NodeId) const; @@ -120,27 +124,32 @@ class DeMorganSimplifier bool process_negated_joined_nodes(NodeId, CsgTree&); // Create an opposite Joined node - Joined build_negated_node(Joined const&) const; + Joined build_negated_node(Joined const&); // Check if this join node should be inserted in the simplified tree bool should_insert_join(NodeId); + // Access or create a translation entry + MatchingNodes& translation(NodeId); + // Find a translation entry, or nullptr if none exist + MatchingNodes const* find_translation(NodeId) const; + //! the tree to simplify CsgTree const& tree_; //! Set when we must insert a \c Negated parent for the given index - std::vector new_negated_nodes_; + NodeSet new_negated_nodes_; //! Set when \c Joined nodes have a \c Negated parent, so we need to insert //! an opposite join node with negated operands - std::vector negated_join_nodes_; + NodeSet negated_join_nodes_; //! Parents matrix. If the pair {e1, e2} exists, e2 is parent of e1 SparseMatrix2D parents_; //! Used during construction of the simplified tree to map replaced nodes //! in the original tree to their new id in the simplified tree - std::vector node_ids_translation_; + NodeMap node_ids_translation_; }; //---------------------------------------------------------------------------// From c5cbc2caf58138deddd62ce13687c5ea082c52ad Mon Sep 17 00:00:00 2001 From: Julien Esseiva Date: Thu, 5 Feb 2026 15:12:11 -0800 Subject: [PATCH 2/2] avoid assertion side-effect --- .../orangeinp/detail/DeMorganSimplifier.cc | 26 +++++++++++-------- 1 file changed, 15 insertions(+), 11 deletions(-) diff --git a/src/orange/orangeinp/detail/DeMorganSimplifier.cc b/src/orange/orangeinp/detail/DeMorganSimplifier.cc index 1850cb8c4e..fd97cfb88f 100644 --- a/src/orange/orangeinp/detail/DeMorganSimplifier.cc +++ b/src/orange/orangeinp/detail/DeMorganSimplifier.cc @@ -217,8 +217,9 @@ CsgTree DeMorganSimplifier::build_simplified_tree() // We're never inserting a negated node pointing to a // joined or negated node so it's child must have an // unmodified equivalent in the simplified tree - CELER_ASSERT(this->translation(negated->node).unmodified); - negated->node = this->translation(negated->node).unmodified; + auto& trans = this->translation(negated->node); + CELER_ASSERT(trans.unmodified); + negated->node = trans.unmodified; } else if (auto* joined = std::get_if(&new_node)) { @@ -229,8 +230,9 @@ CsgTree DeMorganSimplifier::build_simplified_tree() // That means we should find an equivalent node for // each operand, either a simplified negated join or an // unmodified node - CELER_ASSERT(this->translation(op).equivalent_node()); - op = this->translation(op).equivalent_node(); + auto& trans = this->translation(op); + CELER_ASSERT(trans.equivalent_node()); + op = trans.equivalent_node(); } } @@ -261,8 +263,9 @@ CsgTree DeMorganSimplifier::build_simplified_tree() // new tree. // This is not always the exact same node, e.g., if the volume // points to a negated join, it will still be simplified - CELER_ASSERT(this->translation(volume).equivalent_node()); - result.insert_volume(this->translation(volume).equivalent_node()); + auto& trans = this->translation(volume); + CELER_ASSERT(trans.equivalent_node()); + result.insert_volume(trans.equivalent_node()); } return result; @@ -297,9 +300,9 @@ bool DeMorganSimplifier::process_negated_joined_nodes(NodeId node_id, { // Redirect parents looking for this node to the new Joined // node which is logically equivalent - CELER_ASSERT(this->translation(negated->node).opposite_join); - this->translation(node_id).simplified_to - = this->translation(negated->node).opposite_join; + auto& trans = this->translation(negated->node); + CELER_ASSERT(trans.opposite_join); + this->translation(node_id).simplified_to = trans.opposite_join; return false; } @@ -378,8 +381,9 @@ Joined DeMorganSimplifier::build_negated_node(Joined const& joined) { // We should have recorded that this node was necessary // for a join - CELER_ASSERT(this->translation(neg->node).unmodified); - operands.push_back(this->translation(neg->node).unmodified); + auto& trans = this->translation(neg->node); + CELER_ASSERT(trans.unmodified); + operands.push_back(trans.unmodified); } else {