-
-
Notifications
You must be signed in to change notification settings - Fork 454
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
Minor Settings.c cleanup #1531
Minor Settings.c cleanup #1531
Conversation
Rather than open-coding it twice, use a shared routine for deleting columns from the Settings structure. In order to improve code readability refactor the screen array deletion similarly and call both routines from Settings_delete.
Be more flexible and consistent with the way other structures are initialised by passing a pointer to struct Machine into the Settings constructor. This would allow other alternative setup of default Meters in the future, e.g. for some nvtop-alike tool sharing code with htop (hypothetically) showing GPUs instead of CPUs by default. Mainly it just makes the code easier to read.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. Some small suggestions for DiD and consistent code style.
Further small refactor: diff --git a/Settings.c b/Settings.c
index 6ea15d02..555df42f 100644
--- a/Settings.c
+++ b/Settings.c
@@ -190,14 +190,9 @@ static const char* toFieldName(Hashtable* columns, int id, bool* enabled) {
}
if (id >= ROW_DYNAMIC_FIELDS) {
const DynamicColumn* column = DynamicColumn_lookup(columns, id);
- if (!column) {
- if (enabled)
- *enabled = false;
- return NULL;
- }
if (enabled)
- *enabled = column->enabled;
- return column->name;
+ *enabled = column ? column->enabled : false;
+ return column ? column->name : NULL;
}
if (enabled)
*enabled = true; |
571ca63
to
efd05e0
Compare
OK, I've reviewed those changes - I merged the size_t changes into one commit, dropped those things that I felt were closer to "code churn" than "code cleanup", and added a commit for that final suggestion. |
No description provided.