Action/Header/SM/Vector: move const calc expression out of loop#2038
Action/Header/SM/Vector: move const calc expression out of loop#2038GermanAizek wants to merge 1 commit into
Conversation
|
Warning Review limit reached
Next review available in: 28 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Repository UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (4)
📝 WalkthroughWalkthroughChangesThis change caches repeated size and count lookups in loops across Sequence Diagram(s)Not applicable. Estimated code review effort: Medium Suggested labels: refactor, code-quality Suggested reviewers: maintainers familiar with Poem
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: d591b336-4f7c-4a3d-8422-e3671e304828
📒 Files selected for processing (4)
Action.cHeader.cScreenManager.cVector.c
| const int n = this->panelCount; | ||
| if (idx < n) { | ||
| for (int i = idx + 1; i <= n; i++) { |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Inspect Vector_get to determine if it handles out-of-bounds access safely.
ast-grep run --pattern 'Vector_get($_, $_) {
$$$
}' --lang c Vector.c
# Also check for bounds validation in Vector_get
rg -n -A 10 'void\*\s*Vector_get' Vector.c Vector.hRepository: htop-dev/htop
Length of output: 476
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n## Files\n'
git ls-files 'Vector.c' 'Vector.h' 'ScreenManager.c' 'ScreenManager.h'
printf '\n## Vector.c outline\n'
ast-grep outline Vector.c --view expanded || true
printf '\n## Vector.h outline\n'
ast-grep outline Vector.h --view expanded || true
printf '\n## ScreenManager.c relevant lines\n'
sed -n '70,110p' ScreenManager.c | cat -n
printf '\n## Vector_get declaration/definition search\n'
rg -n -A 20 -B 5 'Vector_get|vector_get' Vector.c Vector.h
printf '\n## Vector_insert/Vector_set search\n'
rg -n -A 20 -B 5 'Vector_insert|Vector_set|Vector_size|Vector_get' Vector.c Vector.hRepository: htop-dev/htop
Length of output: 13785
Fix the shift loop bounds in ScreenManager_insert — start at idx and stop before n. i = idx + 1; i <= n skips the panel at idx and reads slot n, which is outside the populated range and can crash or corrupt layout.
GermanAizek
force-pushed
the
const-out-of-loop
branch
2 times, most recently
from
1e62a80 to
3e61aec
Compare
There was a problem hiding this comment.
♻️ Duplicate comments (1)
ScreenManager.c (1)
82-84: 🩺 Stability & Availability | 🔴 Critical | ⚡ Quick winShift loop bounds in
ScreenManager_insertare still incorrect.This was previously flagged but persists. The loop
for (int i = idx + 1; i <= n; i++)has two errors:
- Starts at
idx + 1— skips the panel atidx, which will be shifted right byVector_insertand needs visual adjustment.- Ends at
n(inclusive) — accessesVector_get(this->panels, n)wheren == this->panelCount. Only indices0throughn-1are populated, so indexnis out of bounds and may crash or corrupt layout.Compare with
ScreenManager_remove(line 101) which correctly usesfor (int i = idx; i < n; i++).Fix: adjust both loop bounds
const int n = this->panelCount; if (idx < n) { - for (int i = idx + 1; i <= n; i++) { + for (int i = idx; i < n; i++) { Panel* p = (Panel*) Vector_get(this->panels, i); Panel_move(p, p->x + size, p->y); } }
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: e7aae4cb-b36b-4dfa-ad63-598e1c93648f
📒 Files selected for processing (4)
Action.cHeader.cScreenManager.cVector.c
GermanAizek
force-pushed
the
const-out-of-loop
branch
from
3e61aec to
0b96907
Compare
GermanAizek
force-pushed
the
const-out-of-loop
branch
from
0b96907 to
53143da
Compare
There was a problem hiding this comment.
♻️ Duplicate comments (1)
ScreenManager.c (1)
82-84: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winShift loop bounds in
ScreenManager_insertare still off by one.The loop
for (int i = idx + 1; i <= n; i++)skips the panel atidx(which needs to shift right) and accesses indexn(out of bounds — valid indices are0ton-1).Vector_getperforms no bounds checking, so this can dereference a stale or NULL pointer. The correct loop isfor (int i = idx; i < n; i++).This was previously flagged and remains unfixed.
Fix shift loop bounds
if (idx < n) { - for (int i = idx + 1; i <= n; i++) { + for (int i = idx; i < n; i++) { Panel* p = (Panel*) Vector_get(this->panels, i); Panel_move(p, p->x + size, p->y); } }
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 18942c5c-b617-4e60-af6b-be5f5069de44
📒 Files selected for processing (4)
Action.cHeader.cScreenManager.cVector.c
| Process* process = (Process *)Vector_get(allProcesses, i); | ||
| if (process && Process_getThreadGroup(process) == Process_getThreadGroup(selectedProcess)) { | ||
| const int thgid = Process_getThreadGroup(selectedProcess); | ||
| const int pvecsize = Vector_size(allProcesses); |
There was a problem hiding this comment.
A more readable variable name:
| const int pvecsize = Vector_size(allProcesses); | |
| const int numProcesses = Vector_size(allProcesses); |
| static Htop_Reaction actionUntagAll(State* st) { | ||
| for (int i = 0; i < Panel_size((Panel*)st->mainPanel); i++) { | ||
| Row* row = (Row*) Panel_get((Panel*)st->mainPanel, i); | ||
| Panel* panel = (Panel*)st->mainPanel; |
There was a problem hiding this comment.
We probably don't need a separate variable for (Panel*)st->mainPanel. The pointer stays unchanged during the loop, so the compiler should be able to optimize it.
| for (int j = this->columns[i]->items - 1; j >= 0; j--) { | ||
| const int n = this->columns[i]->items; | ||
| for (int j = n - 1; j >= 0; j--) { |
There was a problem hiding this comment.
Unnecessary change for optimization.
| for (int i = 0; i < Vector_size(this->columns[col]); i++) { | ||
| Meter* meter = (Meter*) Vector_get(this->columns[col], i); | ||
| const Vector* meters = this->columns[col]; | ||
| const int n = Vector_size(meters); |
There was a problem hiding this comment.
Just curious, how can a compiler fail to optimize the Vector_size call here?
Vector_size is an inline function when NDEBUG is defined (i.e. when htop is not built for debug), and Vector_size merely retrieves the items member of the Vector object, which should be a constant throughout the loop.
2 participants
Since I started contributing to heavily optimized AV1 decoder (ffmpeg dav1d), there are high performance requirements. I experimentally found out that putting calculated expressions from the body of the loop into separate constants gives a strong two-fold increase. What is strange is the C/C++ compiler that GCC and Clang on -O3 cannot automatically optimize.
It's also visually more readable with a const separate variable.