Conserve 8 bytes on Event #12
Conversation
WalkthroughThe changes in this pull request focus on modifying the Changes
Possibly related issues
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
include/olga_scheduler/olga_scheduler.hpp (1)
154-154: LGTM! Memory optimization achieved through type change.The change from
std::optional<TimePoint>toTimePointwithTimePoint::min()as sentinel value is an effective optimization that:
- Reduces memory usage by 8 bytes per Event instance
- Potentially improves cache locality
- Eliminates optional-checking overhead
This is a good example of using domain knowledge (TimePoint::min() being a valid sentinel) to optimize memory usage without sacrificing functionality.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
include/olga_scheduler/olga_scheduler.hpp(5 hunks)tests/test_olga_scheduler.cpp(7 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/test_olga_scheduler.cpp
🔇 Additional comments (6)
include/olga_scheduler/olga_scheduler.hpp (6)
84-91: LGTM! Effective optimization of deadline checking.
The change from std::optional to using TimePoint::min() as a sentinel value is a good optimization that maintains the same semantics while reducing memory usage.
118-118: LGTM! Assertion maintains the same invariant.
The assertion correctly validates that the event is unscheduled upon destruction using the new sentinel value approach.
125-125: LGTM! Move constructor maintains proper semantics.
The move constructor correctly handles the deadline transfer and sets the moved-from object's deadline to the sentinel value.
139-139: LGTM! Comparison maintains correct ordering.
The direct comparison maintains the same ordering semantics while being more efficient by avoiding optional access.
326-326: LGTM! Consistent usage of new getDeadline() return type.
The spin() method correctly uses the new direct access pattern, maintaining the same functionality while being more efficient.
98-98: Verify client code compatibility with new getDeadline() return type.
The return type change from std::optional<TimePoint> to TimePoint might affect client code that expects an optional return value.
✅ Verification successful
Let me analyze the search results to verify the impact of the return type change.
Based on the search results, I can now provide the final response:
Return type change from optional to non-optional is safe
The change from std::optional<TimePoint> to TimePoint is consistent with the codebase usage:
- All client code already treats the return value as non-optional, as seen in test assertions and comparisons
- The class invariant (documented above the method) guarantees that events in the tree always have a valid deadline
- Special cases use
TimePoint::min()for canceled/unscheduled events instead of relying on optional
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for getDeadline() usage in client code
rg "getDeadline\(\)" --type cpp -B 2 -A 2
Length of output: 7469
Script:
#!/bin/bash
# Search for the old declaration to confirm the change
rg "std::optional.*getDeadline" --type cpp
Length of output: 44
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (3)
include/olga_scheduler/olga_scheduler.hpp (1)
139-139: Consider extracting TimePoint comparison to a named constant.The comparison could be more readable with a named constant:
- return (dead >= other.deadline_) ? +1 : -1; + static constexpr int LATER = +1, EARLIER = -1; + return (dead >= other.deadline_) ? LATER : EARLIER;tests/test_olga_scheduler.cpp (2)
190-193: Improve assertion readability with custom matchers.Consider creating custom matchers to make the test intentions clearer:
// At the top of the file with other using declarations: using IsScheduled = testing::Gt; using IsNotScheduled = testing::Eq; // Then in the test: EXPECT_THAT(evt_a.getDeadline(), IsScheduled(Loop::time_point::min())) << "Event A should be scheduled"; EXPECT_THAT(evt_c.getDeadline(), IsNotScheduled(Loop::time_point::min())) << "Event C should not be scheduled";
273-292: Add descriptive comments for timing expectations.The test would be clearer with comments explaining the expected timing behavior:
+ // Initial scheduling should be at now + period EXPECT_THAT(evl.getTree()[0U]->getDeadline().time_since_epoch(), 110ms); + // After execution, next deadline should be at execution time + period EXPECT_THAT(evl.getTree()[0U]->getDeadline().time_since_epoch(), 140ms); // Skipped ahead! + // Verify phase error accumulation behavior EXPECT_THAT(evl.getTree()[0U]->getDeadline().time_since_epoch(), 210ms); // Skipped ahead!
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
include/olga_scheduler/olga_scheduler.hpp(5 hunks)tests/test_olga_scheduler.cpp(7 hunks)
🔇 Additional comments (4)
include/olga_scheduler/olga_scheduler.hpp (3)
96-98: Documentation needs to be updated to reflect the new implementation.
The documentation still refers to an optional type, but the implementation has changed to use TimePoint::min() as a sentinel value.
84-91: LGTM: Cancel implementation maintains correct semantics.
The change from optional check to TimePoint::min() comparison is a clean implementation that preserves the original behavior.
326-326: LGTM: Spin implementation correctly uses the new deadline access pattern.
The change simplifies the code by removing the optional handling while maintaining the same functionality.
tests/test_olga_scheduler.cpp (1)
47-48: Consider using more specific test assertions.
The EXPECT_THAT assertions for type ID comparison could be more specific and readable.
- EXPECT_THAT(decltype(evt)::_get_type_id_(), ElementsAreArray(event_type_id));
+ EXPECT_THAT(decltype(evt)::_get_type_id_(), ElementsAreArray(event_type_id))
+ << "Event type ID mismatch";Also applies to: 112-112, 276-276, 305-305
Summary by CodeRabbit
New Features
Bug Fixes