Skip to content

Action/Header/SM/Vector: move const calc expression out of loop #2038

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
GermanAizek wants to merge 1 commit into from
+48 −27
Closed

Action/Header/SM/Vector: move const calc expression out of loop #2038

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
28 changes: 18 additions & 10 deletions Action.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -137,7 +137,8 @@ bool Action_setUserOnly(const char* userName, uid_t* userId) {
static void tagAllChildren(Panel* panel, Row* parent) {
parent->tag = true;
int parent_id = parent->id;
for (int i = 0; i < Panel_size(panel); i++) {
const int n = Panel_size(panel);
for (int i = 0; i < n; i++) {
Row* row = (Row*) Panel_get(panel, i);
if (!row->tag && Row_isChildOf(row, parent_id)) {
tagAllChildren(panel, row);
Expand All @@ -160,7 +161,8 @@ static bool collapseIntoParent(Panel* panel) {
return false;

int parent_id = Row_getGroupOrParent(r);
for (int i = 0; i < Panel_size(panel); i++) {
const int n = Panel_size(panel);
for (int i = 0; i < n; i++) {
Row* row = (Row*) Panel_get(panel, i);
if (row->id == parent_id) {
row->showChildren = false;
Expand Down Expand Up @@ -197,18 +199,19 @@ static Htop_Reaction actionSetSortColumn(State* st) {
Settings* settings = host->settings;
const RowField* fields = settings->ss->fields;
Hashtable* dynamicColumns = settings->dynamicColumns;
const ProcessField activeSortKey = ScreenSettings_getActiveSortKey(settings->ss);
for (int i = 0; fields[i]; i++) {
char* name = NULL;
if (fields[i] >= ROW_DYNAMIC_FIELDS) {
DynamicColumn* column = Hashtable_get(dynamicColumns, fields[i]);
const DynamicColumn* column = Hashtable_get(dynamicColumns, fields[i]);
if (!column)
continue;
name = xStrdup(column->caption ? column->caption : column->name);
} else {
name = String_trim(Process_fields[fields[i]].name);
}
Panel_add(sortPanel, (Object*) ListItem_new(name, fields[i]));
if (fields[i] == ScreenSettings_getActiveSortKey(settings->ss))
if (fields[i] == activeSortKey)
Panel_setSelected(sortPanel, i);

free(name);
Expand Down Expand Up @@ -641,9 +644,11 @@ static Htop_Reaction actionBacktrace(State *st) {

Vector* processes = Vector_new(Class(Process), false, VECTOR_DEFAULT_SIZE);
if (selectedProcess && !Process_isUserlandThread(selectedProcess)) {
for (int i = 0; i < Vector_size(allProcesses); i++) {
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);

@Explorer09 Explorer09 Jul 8, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A more readable variable name:

Suggested change
const int pvecsize = Vector_size(allProcesses);
const int numProcesses = Vector_size(allProcesses);

for (int i = 0; i < pvecsize; i++) {
Process* process = (Process*)Vector_get(allProcesses, i);
if (process && Process_getThreadGroup(process) == thgid) {
Vector_add(processes, process);
}
}
Expand Down Expand Up @@ -819,8 +824,9 @@ static Htop_Reaction actionHelp(State* st) {
// [ ^ 5 ]
// [class1/class2/class3/.../classN used/total]
int barTxtLen = 0;
const bool showCachedMemory = st->host->settings->showCachedMemory;
for (unsigned int i = 0; i < Platform_numberOfMemoryClasses; i++) {
if (!st->host->settings->showCachedMemory && Platform_memoryClasses[i].countsAsCache)
if (!showCachedMemory && Platform_memoryClasses[i].countsAsCache)
continue; // skip reclaimable cache memory classes if "show cached memory" is not ticked
if (!Platform_memoryClasses[i].countsAsUsed && !Platform_memoryClasses[i].countsAsCache)
continue; // skip available memory class (special case for the Linux platform)
Expand Down Expand Up @@ -914,8 +920,10 @@ static Htop_Reaction actionHelp(State* st) {
}

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;

@Explorer09 Explorer09 Jul 8, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

const int n = Panel_size(panel);
for (int i = 0; i < n; i++) {
Row* row = (Row*) Panel_get(panel, i);
row->tag = false;
}
return HTOP_REFRESH;
Expand Down
23 changes: 15 additions & 8 deletions Header.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -66,7 +66,8 @@ void Header_setLayout(Header* this, HeaderLayout hLayout) {
} else {
// move meters from to-be-deleted columns into last one
for (size_t i = newColumns; i < oldColumns; i++) {
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--) {
Comment on lines -69 to +70

@Explorer09 Explorer09 Jul 8, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unnecessary change for optimization.

Vector_add(this->columns[newColumns - 1], Vector_take(this->columns[i], j));
}
Vector_delete(this->columns[i]);
Expand Down Expand Up @@ -182,8 +183,10 @@ Meter* Header_addMeterByClass(Header* this, const MeterClass* type, unsigned int

void Header_reinit(Header* this) {
Header_forEachColumn(this, col) {
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);

@Explorer09 Explorer09 Jul 8, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

for (int i = 0; i < n; i++) {
Meter* meter = (Meter*) Vector_get(meters, i);
if (Meter_initFn(meter)) {
Meter_init(meter);
}
Expand Down Expand Up @@ -213,7 +216,8 @@ void Header_draw(const Header* this) {
roundingLoss -= 1.0F;
}

for (int y = (pad / 2), i = 0; i < Vector_size(meters); i++) {
const int n = Vector_size(meters);
for (int y = (pad / 2), i = 0; i < n; i++) {
Meter* meter = (Meter*) Vector_get(meters, i);

float actualWidth = colWidth;
Expand Down Expand Up @@ -254,11 +258,13 @@ void Header_updateData(Header* this) {
* Returns the number of columns to span, i.e. if the direct neighbor is occupied 1.
*/
static int calcColumnWidthCount(const Header* this, const Meter* curMeter, const int pad, const size_t curColumn, const int curHeight) {
for (size_t i = curColumn + 1; i < HeaderLayout_getColumns(this->headerLayout); i++) {
const size_t max_cols = HeaderLayout_getColumns(this->headerLayout);
for (size_t i = curColumn + 1; i < max_cols; i++) {
const Vector* meters = this->columns[i];

int height = pad;
for (int j = 0; j < Vector_size(meters); j++) {
const int n = Vector_size(meters);
for (int j = 0; j < n; j++) {
const Meter* meter = (const Meter*) Vector_get(meters, j);

if (height >= curHeight + curMeter->h)
Expand All @@ -273,7 +279,7 @@ static int calcColumnWidthCount(const Header* this, const Meter* curMeter, const
}
}

return (int)(HeaderLayout_getColumns(this->headerLayout) - curColumn);
return (int)(max_cols - curColumn);
}

int Header_calculateHeight(Header* this) {
Expand All @@ -284,7 +290,8 @@ int Header_calculateHeight(Header* this) {
Header_forEachColumn(this, col) {
const Vector* meters = this->columns[col];
int height = pad;
for (int i = 0; i < Vector_size(meters); i++) {
const int n = Vector_size(meters);
for (int i = 0; i < n; i++) {
Meter* meter = (Meter*) Vector_get(meters, i);
meter->columnWidthCount = calcColumnWidthCount(this, meter, pad, col, height);
height += meter->h;
Expand Down
15 changes: 9 additions & 6 deletions ScreenManager.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -79,8 +79,9 @@ void ScreenManager_insert(ScreenManager* this, Panel* item, int size, int idx) {
}
Panel_resize(item, size, height);
Panel_move(item, lastX, this->y1 + header_height(this));
if ((size_t)idx < this->panelCount) {
for (int i = idx + 1; (size_t)i <= this->panelCount; i++) {
const int n = this->panelCount;
if (idx < n) {
for (int i = idx + 1; i <= n; i++) {
Comment on lines +82 to +84

@coderabbitai coderabbitai Bot Jul 8, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🩺 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.h

Repository: 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.h

Repository: 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.

Panel* p = (Panel*) Vector_get(this->panels, i);
Panel_move(p, p->x + size, p->y);
}
Expand All @@ -91,12 +92,13 @@ void ScreenManager_insert(ScreenManager* this, Panel* item, int size, int idx) {
}

Panel* ScreenManager_remove(ScreenManager* this, int idx) {
assert((size_t)idx < this->panelCount);
assert(idx < this->panelCount);
int w = ((Panel*) Vector_get(this->panels, idx))->w;
Panel* panel = (Panel*) Vector_remove(this->panels, idx);
this->panelCount--;
if ((size_t)idx < this->panelCount) {
for (size_t i = idx; i < this->panelCount; i++) {
const int n = this->panelCount;
if (idx < n) {
for (int i = idx; i < n; i++) {
Panel* p = (Panel*) Vector_get(this->panels, i);
Panel_move(p, p->x - w, p->y);
}
Expand Down Expand Up @@ -295,7 +297,8 @@ void ScreenManager_run(ScreenManager* this, Panel** lastFocus, int* lastKey, con
ch = KEY_MOUSE_BAR_CLICK;
}
} else {
for (size_t i = 0; i < this->panelCount; i++) {
const size_t n = this->panelCount;
for (size_t i = 0; i < n; i++) {
Panel* panel = (Panel*) Vector_get(this->panels, i);
if (mevent.x >= panel->x && mevent.x <= panel->x + panel->w) {
if (mevent.y == panel->y) {
Expand Down
9 changes: 6 additions & 3 deletions Vector.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,8 @@ Vector* Vector_new(const ObjectClass* type, bool owner, int size) {

void Vector_delete(Vector* this) {
if (this->owner) {
for (int i = 0; i < this->items; i++) {
const int n = this->items;
for (int i = 0; i < n; i++) {
if (this->array[i]) {
Object_delete(this->array[i]);
}
Expand Down Expand Up @@ -82,7 +83,8 @@ int Vector_size(const Vector* this) {
void Vector_prune(Vector* this) {
assert(Vector_isConsistent(this));
if (this->owner) {
for (int i = 0; i < this->items; i++) {
const int n = this->items;
for (int i = 0; i < n; i++) {
if (this->array[i]) {
Object_delete(this->array[i]);
}
Expand Down Expand Up @@ -354,7 +356,8 @@ int Vector_indexOf(const Vector* this, const void* search_, Object_Compare compa
assert(Object_isA(search, this->type));
assert(compare);
assert(Vector_isConsistent(this));
for (int i = 0; i < this->items; i++) {
const int n = this->items;
for (int i = 0; i < n; i++) {
const Object* o = this->array[i];
assert(o);
if (compare(search, o) == 0) {
Expand Down
Loading