feat(FairGenericStack): Provide better default functions #1585
Conversation
WalkthroughWalkthroughThe recent changes involve a comprehensive refactoring of track management within the Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant FairStack
participant FairGenericStack
User->>FairStack: PopNextTrack()
FairStack->>FairGenericStack: GetCurrentTrackNumber()
FairGenericStack-->>FairStack: return current track number
FairStack-->>User: return track data
Possibly related PRs
Suggested reviewers
Recent review detailsConfiguration used: CodeRabbit UI Files selected for processing (7)
Files not reviewed due to no reviewable changes (3)
Files skipped from review as they are similar to previous changes (4)
Additional context usedLearnings (1)
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 (
|
ChristianTackeGSI
force-pushed
the
pr/current_track
branch
2 times, most recently
from
fdffbff to
0753d18
Compare
ChristianTackeGSI
force-pushed
the
pr/current_track
branch
from
0753d18 to
5df348e
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Outside diff range, codebase verification and nitpick comments (5)
templates/project_stl_containers/MyProjData/MyProjStack.h (3)
Line range hint
1-1: Refactoring: Approve the removal ofSetCurrentTrackmethod.The
SetCurrentTrackmethod has been removed as part of the refactoring to centralize track management in the base classFairGenericStack. This change is consistent with the overall design.
Line range hint
1-1: Refactoring: Approve the removal ofGetCurrentTrackNumbermethod.The
GetCurrentTrackNumbermethod has been removed as part of the refactoring to centralize track management in the base classFairGenericStack. This change is consistent with the overall design.
Line range hint
1-1: Refactoring: Approve the removal offCurrentTrackmember variable.The
fCurrentTrackmember variable has been removed as part of the refactoring to centralize track management in the base classFairGenericStack. This change is consistent with the overall design since the base class now handles setting and getting the current track.examples/common/mcstack/FairStack.h (2)
Line range hint
1-1: Refactoring: Approve the removal ofSetCurrentTrackmethod.The
SetCurrentTrackmethod has been removed from theFairStackclass as part of the refactoring to centralize track management in the base classFairGenericStack. Since the base class now provides the implementation, there is no need to override it in the derived class. This change reduces duplication and promotes consistency.
Line range hint
1-1: Refactoring: Approve the removal offCurrentTrackmember variable.The
fCurrentTrackmember variable has been removed from theFairStackclass as part of the refactoring to centralize track management in the base classFairGenericStack. Since the base class now handles storing the current track index, this variable is no longer needed in the derived class. This change reduces duplication and promotes encapsulation.
karabowi
left a comment
karabowi
left a comment
There was a problem hiding this comment.
How does the move of fCurrentTrack from FairStack to FairGenericStack influence
downstream projects, which (for example class PndStack : public FairGenericStack) also declare fCurrentTrack. Will all the project need to get rid of such variable shadowing?
ChristianTackeGSI
force-pushed
the
pr/current_track
branch
from
5df348e to
e754e20
Compare
No, they don't need to change anything. That's why And if they don't like any of this, they can override the methods as they already did. No changes should be necessary. But if they're happy with the "default" implementation, they can strip theirs (like in the templates) and reduce their code sizes. |
ChristianTackeGSI
force-pushed
the
pr/current_track
branch
from
e754e20 to
2c7f459
Compare
FairGenericStack::Reset should reset the appropriate member variables of FairGenericStack. Use this in derived Reset functions.
The templates show a very basic handling of "the current track" in "the Stack". Provide this as a "default implementation" of FairGenericStack. People can still override it with a more sophisticated variant. examples/FairStack just shows that (currently).
ChristianTackeGSI
force-pushed
the
pr/current_track
branch
from
2c7f459 to
23a39ba
Compare
2 participants
Instead of letting each derived "Stack" implement everything, provide some basic functionality in FairGenericStack.
Derived classes can just use it, extend it, or completely replace, as they see fit.
Checklist: