Repository navigation
Fixes for build warnings on macOS with latest toolchain - #1781
Conversation
This resolves warnings observed in macOS CI builds.
a8cdde7 to
7e7a63f
Compare
|
I need to take time to review this. This is obviously a subset of issue #1673. But a few of the changes there require promoting or demoting the data types of certain structures so I didn't start working on that. |
| const int numCols = HeaderLayout_getColumns(this->headerLayout); | ||
| const int width = COLS - 2 * pad - (numCols - 1); | ||
| const size_t numCols = HeaderLayout_getColumns(this->headerLayout); | ||
| const int width = COLS - 2 * pad - ((int)numCols - 1); |
There was a problem hiding this comment.
I don't like this. If numCols needs to be downcast, then numCols should be an unsigned int type. Using size_t would be overkill.
There was a problem hiding this comment.
numCols was previously being downcast implicitly (HeaderLayout_getColumns returns size_t), now it is explicit. The use of an explicit cast here now is to limit the scope of changes to a small, manageable set. I expect many more things will transition to size_t, ideally also in small manageable increments, and these explicit casts will be slowly dropped out of the code over time.
There was a problem hiding this comment.
@natoscott I previously suggested that HeaderLayout_getColumns should return int or unsigned int in order to align with the type limit of terminal columns (COLS). size_t brings no advantage here. If the number of columns in HeaderLayout is greater than COLS then some columns will never be displayed. In other words it makes no sense to allocate so many columns for the Header UI.
There was a problem hiding this comment.
@Explorer09 you're advocating for an unrelated change to HeaderLayout_getColumns - this PR aims for a minimal set of changes to resolve the immediate CI issue, so I'm going to leave this aspect as-is.
7e7a63f to
0919bdd
Compare
|
Changes LGTM for now. I think we can merge this as-is and expand the patch set later. |
| case KEY_CTRL('E'): | ||
| case '$': | ||
| this->scrollH = MAXIMUM(this->selectedLen - this->w, 0); | ||
| this->scrollH = CLAMP((int)this->selectedLen - this->w, 0, INT_MAX); |
There was a problem hiding this comment.
I need to remind that this CLAMP at INT_MAX doesn't really work. Signed integer overflow is undefined behavior, and the compiler can safely assume any (int)x > INT_MAX is false.
There was a problem hiding this comment.
This is an intermediary solution, as the upper limit by ncurses is INT_MAX (based on their API), but when transitioning to size_t later, this will become a proper bound. A proper compiler will optimize this current check out (as unnecessary), but this nonetheless documents the proper, expected limits of the values in the calculation (which is why I suggested this change in the first place).
There was a problem hiding this comment.
@BenBE Yeah, except that Panel.scrollH should probably not migrate to size_t in the first place.
I still find this CLAMP macro call weird. Until I get a big picture about the Panel data structure, I would leave this one as is for now.
There was a problem hiding this comment.
The annoying thing with n curse s is their antiquated API, which has been left behind in the Stone Ages of computer science. So regardless of what you do, you'll end up doing some form of casts to put data to n curse s or retrieve information from the API; but inside htop we should strive to be consistent with anything referencing object counts to use size_t. And that's where things will escalate quickly; e.g. with the Vector API, which is references in many places where also drawing stuff is involved.
There was a problem hiding this comment.
@BenBE No, that wasn't what I mean. I'm aware of "curses" APIs and their limitations. What I had is another API limitation that comes from wcwidth(3) and its family. In particular, I don't think we can make the type of terminal-width-related things anything larger than signed int. Using size_t for Vector sizes is fine. My concern was on members representing terminal width or columns. Because of wcwidth limitation, we're doomed (i.e. using any type larger than int will be futile).
There was a problem hiding this comment.
Yes, and that's exactly, what the intention with the CLAMP is: Use proper types to present the semantics (size_t), while ensuring the proper range (CLAMP) …
I even had a quick look earlier to see if I remember correctly about the limits of the console output stuff (somehow was wondering if it was int16_t or int32_t).
3 participants
No description provided.