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.

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

Revert most of #438 and improve const-correctness #578

Closed
wants to merge 1 commit into from

Conversation

imwints
Copy link
Contributor

@imwints imwints commented Jul 17, 2023

Reverts the removal of const-qualifiers on bools.

Even if this produces the same binary it is good practice to be const-correct and not let developers accidentally change variables that shouldn't be modified

@imwints imwints force-pushed the nb/const-correctness branch 3 times, most recently from a0d160f to ba947cb Compare July 17, 2023 13:12
@stefanos82
Copy link

At least the function parameters keep them as is, they are passed by value, not by const reference or const pointer; therefore the compiler will optimize them out.

@imwints
Copy link
Contributor Author

imwints commented Jul 17, 2023

It is not about what the compiler produces. In fact, the generated binary is identical to the one without const. It is about self documenting code and guarantees.

Take this bit from btop_tools.cpp:

bool refresh(bool only_check) {
	struct winsize w;
	if (ioctl(STDOUT_FILENO, TIOCGWINSZ, &w) < 0) return false;
	if (width != w.ws_col or height != w.ws_row) {
		if (not only_check) {
			width = w.ws_col;
			height = w.ws_row;
		}
	return true;
	}
return false;
}

If you want to know what only_check is for just by reading the declaration you cannot be certain that only_check is just used to control the flow of the function. The value might change depending on the function body and might not be used as you wished when you called the function.

If you see bool refresh(const bool only_check); you can be certain that this is to switch the functions control flow, and only that. The compiler enforces that the value is not changed. It also makes it harder to deviate from the original authors intention for that variable.

If a variable is not intended to be modified it should be declared const to document the code and provide a bit of intention.

It's not ideal that C++ is requiring you to explicitly make variables const unlike rust where mutability is opt-in but we should at least make it harder for everybody to make mistakes and declare as much const as possible

@stefanos82
Copy link

It's not ideal that C++ is requiring you to explicitly make variables const unlike rust where mutability is opt-in but we should at least make it harder for everybody to make mistakes and declare as much const as possible

As I have suspected...well, I'm off; good luck.

@imwints
Copy link
Contributor Author

imwints commented Jul 17, 2023

What's the issue?

@imwints imwints force-pushed the nb/const-correctness branch 2 times, most recently from 93e4df0 to 24fc998 Compare September 13, 2023 15:19
Revert the removal of const qualifiers on bool paramteres and slap const
on all variables that allow it to avoid accidental mutations and improve
code readability
@aristocratos
Copy link
Owner

aristocratos commented Sep 13, 2023

@nobounce
Good effort 👍🏼

But I'm gonna hold off a bit on merging this since it touches code in a lot of places and will cause a lot of messy merges to code still in work in PR's and some branches.
Will probably be less of a pain to merge main in to this PR after the other code has gone in than the other way around.

Have some big improvements planned from a related project I worked on recently.

@imwints
Copy link
Contributor Author

imwints commented Sep 13, 2023

Sounds reasonable, most of this work was done with clang-tidy anyways, so easy to redo (or rebase)

@imwints imwints closed this Jan 2, 2024
@imwints imwints deleted the nb/const-correctness branch January 7, 2024 19:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants