refactor(sysrap): make sflow a scoped enum#418
Conversation
Replace the global flow-control enumerators with a typed sflow enum.\n\nPropagate the scoped type through CSG traversal and QSim action APIs to prevent accidental integer mixing.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5807799532
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
|
||
| QSIM_METHOD void fake_propagate( sphoton& p, const quad2* mock_prd, RNG& rng, unsigned long long idx ); | ||
| QSIM_FORCEINLINE_METHOD int propagate(const int bounce, RNG& rng, sctx& ctx); | ||
| QSIM_FORCEINLINE_METHOD FlowAction propagate(const int bounce, RNG& rng, sctx& ctx); |
There was a problem hiding this comment.
This signature change removes the unscoped START/BREAK names that CSGOptiX/CSGOptiX7.cu still relies on in simulate (int command = START and if(command == BREAK)). In CUDA simulation builds that translation unit includes this header, so the deleted sflow.h no longer supplies those symbols, and sim->propagate(...) now returns a FlowAction that cannot be assigned to the existing int variable. Please update the OptiX bounce loop to use FlowAction::Start/FlowAction::Break too.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Pull request overview
This PR refactors the legacy unscoped flow-control enumerators (UNDEFINED, BREAK, CONTINUE, …) into a strongly-typed scoped enum (FlowAction) and propagates that type through key traversal/propagation APIs (CSG tree traversal and qsim photon propagation) to reduce accidental integer mixing.
Changes:
- Replaces
sysrap/sflow.h(global unscoped enum + CPU-only formatter) withsysrap/FlowAction.h(enum class FlowAction+flow_action_nameformatter). - Updates CSG traversal (
csg_intersect_tree.h) and QSim APIs (qudarap/qsim.h, related tests) to return/useFlowActioninstead ofint. - Removes the old
sflowtest and updates build header lists accordingly.
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| sysrap/tests/sflowTest.cc | Removes the old test tied to the unscoped sflow enum/formatter. |
| sysrap/sflow.h | Deletes the legacy global flow-control enum and CPU formatter. |
| sysrap/FlowAction.h | Adds the new scoped FlowAction enum plus a diagnostic name formatter. |
| sysrap/CMakeLists.txt | Installs/exports FlowAction.h instead of sflow.h. |
| qudarap/tests/QSim_MockTest.cc | Switches boundary-propagation control handling from int to FlowAction and uses flow_action_name. |
| qudarap/qsim.h | Changes propagation routines to return FlowAction and updates return sites/comparisons accordingly. |
| CSG/csg_intersect_tree.h | Replaces unscoped action values with FlowAction in traversal control flow. |
| CSG/csg_classify.h | Removes stale commented-out references to the old shared action enum. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| #pragma once | ||
| #include <string_view> | ||
|
|
| /** | ||
| * Returns the stable diagnostic name of a flow-control action. | ||
| * | ||
| * @param action Flow-control action to format. | ||
| * @return Uppercase enumerator name, or "UNKNOWN" for an invalid value. | ||
| */ | ||
| constexpr std::string_view flow_action_name(FlowAction action) noexcept |
| return "LAST"; | ||
| } | ||
| return "UNKNOWN"; | ||
| } |
Replace the global flow-control enumerators with a typed sflow enum.
Propagate the scoped type through CSG traversal and QSim action APIs to prevent accidental integer mixing.