Upgrade lengths of RichString to size_t & many related changes.

[Explorer09]

The main goal of this PR is to upgrade the "length" parameters and properties of RichString class from int to size_t. While doing the change, there are other code quality improvements that are related or required to the data type migrations.

Important changes of these include:

1. The /proc/*/cmdline basename matching algorithm is slightly changed to utilize the /proc/*/exe matching when possible. (Commit d384376)
2. Add a special case to /proc/*/comm string highlighting. (This is required to prevent an internal variable commStart to go negative.)
3. Convert to size_t for cmdlineBasenameStart, cmdlineBasenameEnd and procExeBasenameOffset values in Process class. (Use size_t for Process offset values #1588)
4. Add additional limit clamping to Panel.cursorX value.
5. Convert to size_t for selectedLen and scrollH values in Panel class. (This incorporates Panel.scrollH clamp logic improvement #1585)
6. Incorporate Fix mbstowcs_nonfatal() that might convert string shorter than desired #1586 and fix mbstowcs_nonfatal(). (It's better for that PR to apply after the RichString lengths migrated to size_t type.)

[BenBE]

There'S a lot of noise introduced due to the (size_t)len casts on RichString_appendnAscii calls, most of which could be avoided if we made xSnprintf return size_t. Can you take a look in that direction?

[BenBE]

Any chance to reduce noise by having size_t len and thus avoiding all the subsequent casts? We know xSnprintf can't be negative, which we already check for.

Suggested change

	len = xSnprintf(buffer, sizeof(buffer), "%5.1f%% ", this->values[CPU_METER_NORMAL]);
	size_t len = (size_t)xSnprintf(buffer, sizeof(buffer), "%5.1f%% ", this->values[CPU_METER_NORMAL]);

[Explorer09]

This approach just moves the casts from the RichString append functions to xSnprintf. Unless we alter the API of xSnprintf, I don't think it reduces any noise overall.

How about this alternate solution: Create a RichString_appendAsciiFormat() function that internally combines xSnprintf and RichString_appendnAscii()?

The prototype could look something like this:

/* The "buf" argument is optional. If "buf" is NULL, then this function uses an internally allocated buffer (using xAsprintf).
It's still recommended to supply "buf" for performance reasons. */
ATTR_NONNULL_N(1, 5)
size_t RichString_appendAsciiFormat(RichString* this, int attrs, char* buf, size_t bufsize, const char* format, ...);

ATTR_NONNULL_N(1, 5)
size_t RichString_appendAsciiVFormat(RichString* this, int attrs, char* buf, size_t bufsize, const char* format, va_list vl);

[BenBE]

Similar consideration re size_t. Maybe update signature of xSnprintf overall?

[BenBE]

Shouldn't the second cast also be size_t?

[Explorer09]

@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.

[BenBE]

This looks overall kinda noisy. The source code shouldn't be a casting show … ;-)

[Explorer09]

That's the problem of policy and whether the compiler would complain about implicit casts to unsigned. Given there is a conditional if (w < 1) return; a few lines earlier, compiler should detect that the w value cannot go negative in this case, but I can't say for sure.

If htop is compiled with gcc or clang's -Wsign-conversion flag, will that cause a warning?

[BenBE]

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

[Explorer09]

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.

[BenBE]

Suggested change

	if (len > (SIZE_MAX - 1) / sizeof(CharType)) {
	if (len > SIZE_MAX / sizeof(CharType) - 1) {

Also, why the special handling here? Other places check only for max number of characters, not max number of bytes for storing these characters.

[Explorer09]

Also, why the special handling here? Other places check only for max number of characters, not max number of bytes for storing these characters.

You don't want the total size in bytes overflow a size_t type, do you?
Because of the +1 on allocated length, I cannot use xMallocArray to simplify things.

[Explorer09]

There'S a lot of noise introduced due to the (size_t)len casts on RichString_appendnAscii calls, most of which could be avoided if we made xSnprintf return size_t. Can you take a look in that direction?

Because snprintf(3) only returns int, the return type that makes more sense for xSnprintf is unsigned int, not size_t.

Even though I can review all calls of xSnprintf and update the return types, I fear that would create more noise for the changes, not less. More importantly, this would take more time of me.