Refactor logic builder and related utilities#2238
Open
sethrj wants to merge 24 commits intoceleritas-project:developfrom
Open
Refactor logic builder and related utilities#2238sethrj wants to merge 24 commits intoceleritas-project:developfrom
sethrj wants to merge 24 commits intoceleritas-project:developfrom
Conversation
Split the builder pattern into two parts: - Policy classes: Immutable factories that can be passed by const reference - Visitor classes: Mutable workers that hold references to the logic vector This makes the code clearer by separating configuration (policy) from execution (visitor). The policy's operator() creates a visitor with the necessary references, and build_logic coordinates the process. Benefits: - Policies are now truly immutable and can be reused - No more confusing move semantics or rvalue references - Clear separation between factory and worker - Maintains the same CRTP pattern for postfix/infix variants Assisted-by: GitHub Copilot (Claude Sonnet 4.5)
sethrj
added
minor
Test summary 5 893 files 9 431 suites 18m 39s ⏱️ Results for commit f736bf5. ♻️ This comment has been updated with latest results. |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## develop #2238 +/- ##
===========================================
+ Coverage 86.81% 86.83% +0.01%
===========================================
Files 1330 1330
Lines 42019 42048 +29
Branches 12950 12959 +9
===========================================
+ Hits 36480 36511 +31
+ Misses 4335 4334 -1
+ Partials 1204 1203 -1
🚀 New features to boost your workflow:
|
This reverts commit 167896a.
sethrj
commented
Feb 6, 2026
| // Calculate the maximum stack depth of the volume definition | ||
| int depth = calc_depth(input_logic); | ||
| // TODO: is this valid for infix?? | ||
| int depth = calc_depth(make_span(v.logic)); |
Member
Author
There was a problem hiding this comment.
@esseivaju I think this is only needed for postfix runtime?
| // SPDX-License-Identifier: (Apache-2.0 OR MIT) | ||
| //---------------------------------------------------------------------------// | ||
| //! \file orange/orangeinp/ScaleUtils.cc | ||
| //! \file orange/detail/LogicIO.hh |
Member
Author
There was a problem hiding this comment.
Ignore the faulty git rename detection
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This is in advance of some fixes for the infix/postfix builder/converter, which is somewhat confusing because of the hardcoded posfix construction (but with simplification) despite the hardcoded infix tracking.