Code improvement wherever possible

[stefanos82]

I have tried to find real-time contact information such as Discord or IRC channel but could not find anything, therefore I will ask here if it's alright.

I have noticed some coding "peculiarities" as I like to call them that could be replaced with improved code.

Is it OK to fork, make the necessary changes, and then PR or do I need to follow a certain procedure first?

Please advise.

Thank you very much.

[aristocratos]

@stefanos82
No procedures needed, just follow the guidelines and formatting specified in CONTRIBUTING.md.

Note that btop++ was the project I used to learn C++, so there's likely to be a lot of inconsistencies in the efficiency of the code (and a lot of opportunities for optimization).

If the coding "peculiarities" you mention is about style and not function you would also need a good argument to why the code should be changed (that's not based on just personal opinion).

[aristocratos]

Maybe clearer names for each color, much like those fmt is using: https://fmt.dev/latest/api.html#terminal-color-and-text-style

Considering the names used now are the names for the theme defined 24-bit colors. "main_fg" being the main foreground text color, so not sure what clarity would be gained by not matching the same names as in the .theme files.

What I meant, was at the start of functions that use a lot of the same theme colors, put something like:

const auto& main_fg = Theme::c("main_fg); //Main foreground color
const auto& hi_fg = Theme::c("hi_fg"); //Highlight foreground color
const auto& hi_bg = Theme::c("hi_bg"); //Highlight background color

and so on, to get rid of all the function calls in the strings.

[stefanos82]

Ah OK, got it.

[stefanos82]

What I meant, was at the start of functions that use a lot of the same theme colors, put something like:

const auto& main_fg = Theme::c("main_fg); //Main foreground color
const auto& hi_fg = Theme::c("hi_fg"); //Highlight foreground color
const auto& hi_bg = Theme::c("hi_bg"); //Highlight background color

and so on, to get rid of all the function calls in the strings.

What I wanted to ask is why did you even created the inline function anyway?

You could achieve the same result with auto hi_bg = Theme::colors["hi_bg"]; and save an unnecessary function call.

I guess you didn't notice that during the initial time of the original implementation.

[aristocratos]

Not sure actually, I believe the thinking at the time might have been to at the start of functions use using Theme; and then just call c("main_fg") to shorten it, which it turns out I forgot about...

[stefanos82]

Should we introduce a nested namespace in Theme named Color so we can include the color names in there?

Something like Theme::Color::hi_bg so we can use namespace alias: namespace tc = Theme::Color; thus to cleaner usage, like tc::hi_bg;, but of course a better name would make a lot more sense than tc.

[stefanos82]

By the way, since GCC 12.1 we can use -fuse-ld= flag to choose a different linker when we want to.

I personally use mold combined with ccache which improves compilation-times sometimes by orders of magnitude!

Any feedback on that?

[aristocratos]

Well, compile times haven't really been any issue in this project, pretty sure most x86 systems from the last decade or so are gonna build btop in under 1 minute.

Anyone wanting to build with non default linker can just set the desired flags when calling make, so no issues there really.

[stefanos82]

Oh that's the usage of ADDFLAGS, very clever! 😃

[stefanos82]

Currently I'm working on replacing const bool& to bool.

Reminder: Boolean literals false and true are prvalues of type bool, therefore we trust the compiler to do its magics with optimization(s).

Question: What exactly is this boolean variable doing in btop_draw.cpp, named show_temp? I don't understand 🤔 , is your intention to use it as 1 in case it's true?

		//* Calculate and draw cpu box outlines
		if (Cpu::shown) {
			using namespace Cpu;
            bool show_temp = (Config::getB("check_temp") and got_sensors);
			width = round((double)Term::width * width_p / 100);
			height = max(8, (int)ceil((double)Term::height * (trim(boxes) == "cpu" ? 100 : height_p) / 100));
			x = 1;
			y = cpu_bottom ? Term::height - height + 1 : 1;

			b_columns = max(1, (int)ceil((double)(Shared::coreCount + 1) / (height - 5)));
			if (b_columns * (21 + 12 * show_temp) < width - (width / 3)) {
				b_column_size = 2;
				b_width = (21 + 12 * show_temp) * b_columns - (b_columns - 1);
			}
			else if (b_columns * (15 + 6 * show_temp) < width - (width / 3)) {
				b_column_size = 1;
				b_width = (15 + 6 * show_temp) * b_columns - (b_columns - 1);
			}
			else if (b_columns * (8 + 6 * show_temp) < width - (width / 3)) {
				b_column_size = 0;
			}
			else {
				b_columns = (width - width / 3) / (8 + 6 * show_temp);
				b_column_size = 0;
			}

			if (b_column_size == 0) b_width = (8 + 6 * show_temp) * b_columns + 1;
			b_height = min(height - 2, (int)ceil((double)Shared::coreCount / b_columns) + 4);
                        ...
                        ...

[aristocratos]

Question: What exactly is this boolean variable doing in btop_draw.cpp, named show_temp? I don't understand 🤔 , is your intention to use it as 1 in case it's true?

Haven't seen that before? I guess it's more common in python.
Instead of an ternary statement just use the bool to either negate or keep the values depending on if true (1) or false (0).

[stefanos82]

I guess

bool show_temp = (Config::getB("check_temp") and got_sensors);

should be replaced with

int show_temp = (Config::getB("check_temp") and got_sensors) ? 1 : 0;

to make it more obvious of what it supposed to do.

[aristocratos]

Or just a comment to explain the behavior, I thought this usage of bools was common knowledge.

[stefanos82]

I can honestly say I have never seen such code in C++, thus my surprise when I saw it.

[stefanos82]

I'm currently working on btop_theme.cpp and we have the following code:

namespace {
	//* Convert 24-bit colors to 256 colors
	int truecolor_to_256(const int& r, const int& g, const int& b) {
		//? Use upper 232-255 greyscale values if the downscaled red, green and blue are the same value
		if (const int red = round((double)r / 11); red == round((double)g / 11) and red == round((double)b / 11)) {
			return 232 + red;
		}
		//? Else use 6x6x6 color cube to calculate approximate colors
		else {
			return round((double)r / 51) * 36 + round((double)g / 51) * 6 + round((double)b / 51) + 16;
		}
	}
}

I have an idea how to get rid of the C-style type casting and I want your feedback.

Here's how the code will look:

namespace {
	//* Convert 24-bit colors to 256 colors
	int truecolor_to_256(const int& r, const int& g, const int& b) {
		//? Use upper 232-255 greyscale values if the downscaled red, green and blue are the same value
		if (const int red = round((r+0.0) / 11); red == round((g+0.0) / 11) and red == round((b+0.0) / 11)) {
			return 232 + red;
		}
		//? Else use 6x6x6 color cube to calculate approximate colors
		else {
			return round((r+0.0) / 51) * 36 + round((g+0.0) / 51) * 6 + round((b+0.0) / 51) + 16;
		}
	}
}

As you can see, we are taking an advantage of implicit conversion mechanism and since we are using round(), we know it expects either of the following types: float, double, long double, or IntegralType (I hope I see well here https://en.cppreference.com/w/cpp/numeric/math/round).

So, what do you think?

[stefanos82]

I personally like to use any modern C++ type cast only when it's necessary; if the compiler offers me a different way to convert to a type I'm interested in, then I prefer that method.

As I have said, C-style type casts when combined with C++'s reinterpret_cast could lead to trouble which is something we want to avoid at all costs.

[aristocratos]

As I have said, C-style type casts when combined with C++'s reinterpret_cast could lead to trouble which is something we want to avoid at all costs.

Sure, but that wouldn't apply here since the cast is to a temporary created inside round() so the cast is never captured by a variable that can be reinterpret_casted.

[stefanos82]

Fair enough; I won't apply those changes then.

[aristocratos]

Well to me it doesn't really matter as long as it works 😉
I'm just arguing that it wouldn't give better readability compared to the C-style cast and there is no UB to guard against (that I can see) in this particular case.

[stefanos82]

By the way, I have tested 3 separate scenarios of casting a variable and produced the same assembly output:

int main()
{
    int i = 11;
    // return static_cast<float>(i);
    // return i+0.0f;
    return (float)i; 
}

Assembly output:

main:
        push    rbp
        mov     rbp, rsp
        mov     DWORD PTR [rbp-4], 11
        pxor    xmm0, xmm0
        cvtsi2ss        xmm0, DWORD PTR [rbp-4]
        cvttss2si       eax, xmm0
        pop     rbp
        ret

Pay close attention in C++'s comment section of f suffix; that little detail produces two different assembly commands when we forget to add it to 0.0 in case we are dealing with float values and not double:

int main()
{
    int i = 11;
    return i+0.0;
}

Assembly Output:

main:
        push    rbp
        mov     rbp, rsp
        mov     DWORD PTR [rbp-4], 11
        pxor    xmm0, xmm0
        cvtsi2sd        xmm0, DWORD PTR [rbp-4]
        cvttsd2si       eax, xmm0
        pop     rbp
        ret

It converts it to a double-precision floating-point value whereas the previous cases are of single-precision floating-point value; tiny detail yet so important!

I learned something new today 😃

P.S.: Don't get fooled that C-style typecasting and C++ typecasting work the same, just because they produced the same assembly output; prefer C++'s typecasting when needed as it has more compile-time checking mechanisms than C's.