-
-
Notifications
You must be signed in to change notification settings - Fork 185
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
Major cleanup #1858
Major cleanup #1858
Conversation
cd6d6e2
to
3e6c723
Compare
f0888fe
to
e233c5f
Compare
f5f27ae
to
5ed3c3d
Compare
I really like how this is evolving! Just as a heads up: IMO this PR gets too huge and must be split up into multiple ones. But only when you're finished and I guess the best would be 1 PR per commit as they're already separating the topics. (Minus the fixup commit(s) which should be squashed into the appropriate topic ones). 👍 |
f30b935
to
8c8419c
Compare
Since it's going to be split up, I will keep it "drafted" and then close/something. Please, don't review this one. |
I'm not quite certain what's the plan here. A commit should do one thing. For example:
is doing a lot of unrelated things. And I don't quite see why they are in one commit. This makes it hard to review. To pick and choose changes. Similar for:
I think you need a commit
something like this. I'm not sure if i missed something because really its hard to read :) And for me it would be okay if such commits would be in one cleanup PR. But sure if this touches too many lines/code then it can also help to split it to several PRs. What I mean is that the one atomic change per commit is the more important splitting. |
For example in #1863 you now have only one commit. But you do 2 things there:
These things are not related and should be separate commits. |
f30b935
to
830d152
Compare
Your other PR got merged. First commit can be dropped I guess. |
830d152
to
946eee6
Compare
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.
Again please dont mix in one commit several things that dont belong together.
Split out the change int he first (two?) commits.
And put the description of the functions above the functions in the .c file.
Also please be aware that we decided in #1859 that we don't want doxygen comments.
So far I accepted them in cases where I believed that our function name is really not descriptive enough and your comments were a good explanation.
I would still prefer them written in another way but its also fine by me to have them like this. But the goal will not be to have doxygen comments for each function.
And please update the first comment in this PR to reflect the state of it.
* | ||
* @note The returned value must be freed using g_free when it is no longer needed | ||
* to prevent memory leaks. | ||
*/ |
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.
Comment about function should be in the .c file and not the header.
The dev is reading the .c file and shouldnt have to switch to the header just to read the comment.
Maybe some IDEs take it from the .h file but I'm not a fan of this.
True for all other cases in this commit as well.
And please break up this commit. The docu change should be a commit of its own. And not mixed with the char one.
Improve usage of `gchar` and `char` by setting them correctly Increase usage of `auto_gchar` and `auto_char` Fix 2 mem leaks (rosterwin.c, avatar.c)
946eee6
to
69bb2d8
Compare
Fix 11 potential mem leaks in theme.c
Remove unused variables Apply minor cleanups
Include some additional minor cleanups
8861cb0
to
a52995d
Compare
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.
Looks good! But there are still function description comments in one .h file :)
it was for macros, since they are used only in .h file |
a52995d
to
09f217d
Compare
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.
Huge PR to review. I hope I didn't miss anything. LGTM.
void | ||
prefs_set_string(preference_t pref, char* value) | ||
prefs_set_string(preference_t pref, gchar* new_value) |
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.
I think value
was quite descriptive ;)
gchar* | ||
prefs_get_string_with_option(preference_t pref, gchar* option) |
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.
It looks like we even don't use this function anymore?
I see! My bad! |
The changes are intended to fix most of the cases of #1819 (incorrect usage of
gchar
instead ofchar
and vice versa leading to incorrect memoryfree
ing).prefs_get_string
asgchar
auto_gchar
instead ofg_free
when suitable.