SeekHandler: Some janitoring #26794
SeekHandler: Some janitoring #26794
Conversation
c1ef86b to
a972e76
Compare
There was a problem hiding this comment.
Pull Request Overview
Modernizes and simplifies the SeekHandler implementation by adopting modern C++ constructs and removing redundant code.
- Replaces manual container insertions and loops with
emplace,std::ranges::for_each,std::shift_left, andstd::accumulate - Converts C-style arrays and raw pointers to
std::arrayandnullptr, and addsconstqualifiers - Removes explicit destructor and cleans up includes
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| xbmc/SeekHandler.h | Swapped raw array for std::array, default-initialized members, removed destructor declaration, updated includes |
| xbmc/SeekHandler.cpp | Replaced manual clears and inserts with emplace, modernized loops with range algorithms, improved const usage |
Comments suppressed due to low confidence (1)
xbmc/SeekHandler.cpp:422
- [nitpick] Consider adding unit tests for the time code rotation (
ChangeTimeCode) and parsing logic (GetTimeCodeSeconds) to catch regressions introduced by the switch tostd::shift_leftandstd::accumulate.
if (m_timeCodePosition > 0)
xbmc/SeekHandler.cpp
Outdated
| for (int i = 0; i < m_timeCodePosition; i++) | ||
| tot = tot * 10 + m_timeCodeStamp[i]; | ||
| int tot = | ||
| std::accumulate(m_timeCodeStamp.begin(), m_timeCodeStamp.end(), 0, |
There was a problem hiding this comment.
The accumulation iterates over all 6 slots (including unfilled zeros), producing incorrect values when fewer than 6 digits have been entered. Limit the range to the first m_timeCodePosition elements (e.g. begin() to begin() + m_timeCodePosition).
| std::accumulate(m_timeCodeStamp.begin(), m_timeCodeStamp.end(), 0, | |
| std::accumulate(m_timeCodeStamp.begin(), m_timeCodeStamp.begin() + m_timeCodePosition, 0, |
Copilot uses AI. Check for mistakes.
xbmc/SeekHandler.cpp
Outdated
| std::ranges::for_each(seekTypeSettingMap, | ||
| [&settings, &forwardSteps = m_forwardSeekSteps, | ||
| &backwardSteps = m_backwardSeekSteps](const auto& it) | ||
| { | ||
| auto& forward = forwardSteps[it.first]; | ||
| auto& backward = backwardSteps[it.first]; | ||
|
|
||
| std::ranges::for_each(settings->GetList(it.second), | ||
| [&backward, &forward](const auto& seekStep) | ||
| { | ||
| const int stepSeconds = | ||
| static_cast<int>(seekStep.asInteger()); | 10000||
| if (stepSeconds < 0) | ||
| backward.insert(backward.begin(), stepSeconds); | ||
| else | ||
| forward.push_back(stepSeconds); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
[nitpick] This nested lambda structure with an ambiguously named parameter (it) and deep nesting hurts readability. Consider using a simple range-based for loop or renaming the lambda parameter to something more descriptive (e.g. entry).
Copilot uses AI. Check for mistakes.
and reduce number of copies performed
a972e76 to
a9b38db
Compare
There was a problem hiding this comment.
Pull Request Overview
This PR modernizes and simplifies the SeekHandler code by leveraging modern C++ features. Key changes include:
- Replacing raw arrays with std::array and providing default initializations.
- Refactoring loops with C++20 algorithms like std::ranges::for_each and std::shift_left.
- Improving const correctness and updating legacy pointer comparisons.
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| xbmc/SeekHandler.h | Uses std::array for the time code stamps and removes unnecessary code |
| xbmc/SeekHandler.cpp | Employs modern iteration and accumulation techniques, and updates error comparisons |
Comments suppressed due to low confidence (1)
xbmc/SeekHandler.cpp:58
- Verify that the use of C++20 ranges, specifically std::ranges::for_each, aligns with the project's minimum C++ standard requirements.
std::ranges::for_each(seekTypeSettingMap, [&settings, &forwardSteps = m_forwardSeekSteps, &backwardSteps = m_backwardSeekSteps](const auto& seekDef) {
| @@ -418,8 +409,7 @@ void CSeekHandler::ChangeTimeCode(int remote) | |||
| else | |||
| { | |||
| // rotate around | |||
| for (int i = 0; i < 5; i++) | |||
| m_timeCodeStamp[i] = m_timeCodeStamp[i + 1]; | |||
| std::shift_left(m_timeCodeStamp.begin(), m_timeCodeStamp.end(), 1); | |||
There was a problem hiding this comment.
[nitpick] Consider adding a brief comment to explain the use of std::shift_left for rotating the timestamp array, noting its C++20 origin to assist future maintainers.
Copilot uses AI. Check for mistakes.
Description
Modernization and simplification of some SeekHandler code
Motivation and context
More modern, more good
How has this been tested?
It builds
What is the effect on users?
None
Types of change
Checklist: