Skip to content
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

Upgrade lengths of RichString to size_t & many related changes. #1592

Draft
wants to merge 22 commits into
base: main
Choose a base branch
from
+458 −339
Draft

Upgrade lengths of RichString to size_t & many related changes. #1592

Show file tree
Hide file tree
Changes from 16 commits
Commits
Show all changes
22 commits
Select commit Hold shift + click to select a range
233c953
Fix potential out of bound access in PCPProcessTable_updateCmdline()
Explorer09 Jan 15, 2025
160c066
Minor code style adjustment in matchCmdlinePrefixWithExeSuffix()
Explorer09 Jan 16, 2025
f83ab04
Improve process cmdline basename matching with procExe path
Explorer09 Jan 16, 2025
56654dd
Improve "comm" string highlighting in Process_makeCommandStr()
Explorer09 Jan 16, 2025
9a395cb
Process: Use size_t for "cmdline" and "procExe" offsets
Explorer09 Jan 16, 2025
9eb2343
Simplify an expression in Process_updateExe()
Explorer09 Jan 16, 2025
473c847
Simplify findCommInCmdline() logic in Process.c
Explorer09 Jan 16, 2025
7ff906c
RichString_setLen() minor logic fix
Explorer09 Jan 18, 2025
bd27b7e
Prevent length wraparound in RichString_rewind()
Explorer09 Jan 18, 2025
1715ad0
Replace unnecessary RichString_appendnAscii() calls with appendAscii()
Explorer09 Jan 18, 2025
4ef38cb
Use RichString_appendnAscii() for CPU frequency text
Explorer09 Jan 18, 2025
fd54355
Add an assertion to Row_printLeftAlignedField()
Explorer09 Jan 18, 2025
39c14cd
Add bound checks when changing Panel.cursorX value
Explorer09 Jan 18, 2025
521e563
Remove redundant length clamping in Panel_draw()
Explorer09 Jan 18, 2025
78184a1
Panel.scrollH value clamp logic improvement
Explorer09 Jan 18, 2025
e101339
Use size_t for Panel.scrollH and Panel.selectedLen
Explorer09 Jan 18, 2025
9af21a7
Upgrade all length parameters of RichString class to size_t
Explorer09 Jan 18, 2025
4b32769
Add size limit checks to RichString methods
Explorer09 Jan 20, 2025
5d61426
Improve BarMeterMode_draw() limit check after data type changes
Explorer09 Jan 18, 2025
22deed3
Improve LEDMeterMode_draw() limit check after data type changes
Explorer09 Jan 18, 2025
1b8f1ab
Fix mbstowcs_nonfatal() that might convert string shorter than desired
Explorer09 Jan 18, 2025
005c9b6
Introduce RichString_append{,n}FormatAscii functions
Explorer09 Jan 20, 2025
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
2 changes: 1 addition & 1 deletion CPUMeter.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -187,7 +187,7 @@ static void CPUMeter_display(const Object* cast, RichString* out) {
len = xSnprintf(cpuFrequencyBuffer, sizeof(cpuFrequencyBuffer), "N/A ");
}
RichString_appendAscii(out, CRT_colors[METER_TEXT], "freq: ");
RichString_appendnWide(out, CRT_colors[METER_VALUE], cpuFrequencyBuffer, len);
RichString_appendnAscii(out, CRT_colors[METER_VALUE], cpuFrequencyBuffer, len);
}

#ifdef BUILD_WITH_CPU_TEMP
Expand Down
2 changes: 1 addition & 1 deletion CRT.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -825,7 +825,7 @@ static int CRT_colorSchemes[LAST_COLORSCHEME][LAST_COLORELEMENT] = {

static bool CRT_retainScreenOnExit = false;

int CRT_scrollHAmount = 5;
unsigned int CRT_scrollHAmount = 5;

int CRT_scrollWheelVAmount = 10;

Expand Down
2 changes: 1 addition & 1 deletion CRT.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -190,7 +190,7 @@ extern const int* CRT_colors;

extern int CRT_cursorX;

extern int CRT_scrollHAmount;
extern unsigned int CRT_scrollHAmount;

extern int CRT_scrollWheelVAmount;

Expand Down
3 changes: 2 additions & 1 deletion MainPanel.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@ in the source distribution for its full text.
#include "MainPanel.h"

#include <ctype.h>
#include <stddef.h>
#include <stdlib.h>
#include <sys/types.h>

Expand Down Expand Up @@ -81,7 +82,7 @@ static HandlerResult MainPanel_eventHandler(Panel* super, int ch) {

if (EVENT_IS_HEADER_CLICK(ch)) {
int x = EVENT_HEADER_CLICK_GET_X(ch);
int hx = super->scrollH + x + 1;
size_t hx = super->scrollH + (unsigned int)x + 1;
Copy link
Member

Choose a reason for hiding this comment

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

Shouldn't the second cast also be size_t?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@BenBE Let me explain a bit:

  • signed int cast to size_t: Perform a sign extension.
  • unsigned int cast to size_t: Perform a zero extension.

When I made this patch, I made a personal rule that, if a zero extension would suffice, I would avoid sign extension. This also applies to the noisy-as-you-called-it changes about me casting the xSnprintf results to size_t.

RowField field = RowField_keyAt(settings, hx);
if (ss->treeView && ss->treeViewAlwaysByPID) {
ss->treeView = false;
Expand Down
45 changes: 30 additions & 15 deletions Panel.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@ in the source distribution for its full text.

#include <assert.h>
#include <ctype.h>
#include <limits.h>
#include <stdbool.h>
#include <stdlib.h>
#include <string.h>
Expand Down Expand Up @@ -79,7 +80,16 @@ void Panel_done(Panel* this) {

void Panel_setCursorToSelection(Panel* this) {
this->cursorY = this->y + this->selected - this->scrollV + 1;
this->cursorX = this->x + this->selectedLen - this->scrollH;

int offset = this->w;
if (this->selectedLen - this->scrollH < (unsigned int)this->w) {
offset = (int)(this->selectedLen - this->scrollH);
}
if ((unsigned int)offset <= INT_MAX - (unsigned int)this->x) {
this->cursorX = this->x + offset;
} else {
this->cursorX = INT_MAX;
}
}

void Panel_setSelectionColor(Panel* this, ColorElements colorId) {
Expand Down Expand Up @@ -222,7 +232,7 @@ void Panel_draw(Panel* this, bool force_redraw, bool focus, bool highlightSelect
assert (this != NULL);

int size = Vector_size(this->items);
int scrollH = this->scrollH;
size_t scrollH = this->scrollH;
int y = this->y;
int x = this->x;
int h = this->h;
Expand All @@ -239,13 +249,12 @@ void Panel_draw(Panel* this, bool force_redraw, bool focus, bool highlightSelect
else
RichString_setAttr(&this->header, header_attr);
}
int headerLen = RichString_sizeVal(this->header);
size_t headerLen = RichString_sizeVal(this->header);
if (headerLen > 0) {
attrset(header_attr);
mvhline(y, x, ' ', this->w);
if (scrollH < headerLen) {
RichString_printoffnVal(this->header, y, x, scrollH,
MINIMUM(headerLen - scrollH, this->w));
RichString_printoffnVal(this->header, y, x, scrollH, this->w);
}
attrset(CRT_colors[RESET_COLOR]);
y++;
Expand Down Expand Up @@ -282,8 +291,7 @@ void Panel_draw(Panel* this, bool force_redraw, bool focus, bool highlightSelect
const Object* itemObj = Vector_get(this->items, i);
RichString_begin(item);
Object_display(itemObj, &item);
int itemLen = RichString_sizeVal(item);
int amt = MINIMUM(itemLen - scrollH, this->w);
size_t itemLen = RichString_sizeVal(item);
if (highlightSelected && i == this->selected) {
item.highlightAttr = selectionColor;
}
Expand All @@ -293,8 +301,8 @@ void Panel_draw(Panel* this, bool force_redraw, bool focus, bool highlightSelect
this->selectedLen = itemLen;
}
mvhline(y + line, x, ' ', this->w);
if (amt > 0)
RichString_printoffnVal(item, y + line, x, scrollH, amt);
if (scrollH < itemLen)
RichString_printoffnVal(item, y + line, x, scrollH, this->w);
if (item.highlightAttr)
attrset(CRT_colors[RESET_COLOR]);
RichString_delete(&item);
Expand All @@ -309,22 +317,22 @@ void Panel_draw(Panel* this, bool force_redraw, bool focus, bool highlightSelect
const Object* oldObj = Vector_get(this->items, this->oldSelected);
RichString_begin(old);
Object_display(oldObj, &old);
int oldLen = RichString_sizeVal(old);
size_t oldLen = RichString_sizeVal(old);
const Object* newObj = Vector_get(this->items, this->selected);
RichString_begin(new);
Object_display(newObj, &new);
int newLen = RichString_sizeVal(new);
size_t newLen = RichString_sizeVal(new);
this->selectedLen = newLen;
mvhline(y + this->oldSelected - first, x + 0, ' ', this->w);
if (scrollH < oldLen)
RichString_printoffnVal(old, y + this->oldSelected - first, x,
scrollH, MINIMUM(oldLen - scrollH, this->w));
scrollH, this->w);
attrset(selectionColor);
mvhline(y + this->selected - first, x + 0, ' ', this->w);
RichString_setAttr(&new, selectionColor);
if (scrollH < newLen)
RichString_printoffnVal(new, y + this->selected - first, x,
scrollH, MINIMUM(newLen - scrollH, this->w));
scrollH, this->w);
attrset(CRT_colors[RESET_COLOR]);
RichString_delete(&new);
RichString_delete(&old);
Expand Down Expand Up @@ -378,7 +386,11 @@ bool Panel_onKey(Panel* this, int key) {
case KEY_LEFT:
case KEY_CTRL('B'):
if (this->scrollH > 0) {
this->scrollH -= MAXIMUM(CRT_scrollHAmount, 0);
if (this->scrollH > CRT_scrollHAmount) {
this->scrollH = this->scrollH - CRT_scrollHAmount;
} else {
this->scrollH = 0;
}
Comment on lines +389 to +393
Copy link
Contributor Author

@Explorer09 Explorer09 Jan 28, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Note to self: This may be changed to a saturatingSub call if there is no missed optimization.

this->needsRedraw = true;
}
break;
Expand Down Expand Up @@ -421,7 +433,10 @@ bool Panel_onKey(Panel* this, int key) {

case KEY_CTRL('E'):
case '$':
this->scrollH = MAXIMUM(this->selectedLen - this->w, 0);
this->scrollH = 0;
if (this->selectedLen > (unsigned int)this->w) {
this->scrollH = this->selectedLen - (unsigned int)this->w;
}
this->needsRedraw = true;
break;

Expand Down
5 changes: 3 additions & 2 deletions Panel.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ in the source distribution for its full text.

#include <assert.h>
#include <stdbool.h>
#include <stddef.h>

#include "CRT.h"
#include "FunctionBar.h"
Expand Down Expand Up @@ -67,10 +68,10 @@ struct Panel_ {
Vector* items;
int selected;
int oldSelected;
int selectedLen;
size_t selectedLen;
void* eventHandlerState;
int scrollV;
Copy link
Member

Choose a reason for hiding this comment

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

For consistency, this would need to change to size_t too.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I was considering that changing the scrollH type to size_t would be a bad move. It should have been unsigned int instead. It doesn't make sense to allow the UI to display 2 GiB of characters nor allow user to scroll past 2 GiB characters. In other words, it might be better to cap the scrollH value to INT_MAX whenever we have such a large data.

Copy link
Member

Choose a reason for hiding this comment

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

Capping the internal value is independent from the data type used (to some extend).

Also, the marked line is about scrollV, not scrollH.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

What I was considering is setting the data type of scrollH to unsigned int. And for consistency change scrollV to unsigned int as well. Even though I consider the scrollV changes be separate commits (because it's not directly related to or depend on RichString).

int scrollH;
size_t scrollH;
bool needsRedraw;
bool cursorOn;
bool wasFocus;
Expand Down
Loading