Major cleanup #1858

H3rnand3zzz · 2023-06-15T16:36:44Z

The changes are intended to fix most of the cases of #1819 (incorrect usage of gchar instead of char and vice versa leading to incorrect memory freeing).

Fix memory leaks/remove markers in a new commit
Fix double free's introduced in changes
Second commit for prefs_get_string as gchar
Third commit auto_gchar instead of g_free when suitable.
Fourth commit for jids
Fifth commit for gcharv
Arch check
Self-review

sjaeckel · 2023-06-29T08:18:11Z

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). 👍
... But maybe I'm also telling you what you had already planned to do 😆

H3rnand3zzz · 2023-07-04T16:32:53Z

Since it's going to be split up, I will keep it "drafted" and then close/something. Please, don't review this one.

jubalh · 2023-07-05T09:25:24Z

I'm not quite certain what's the plan here.
But if this is going to be split then the most important part should be this:

A commit should do one thing. For example:

[Cleanup prefs_get_string](https://github.com/profanity-im/profanity/pull/1858/commits/b57d9a5ef022cb4c2d39cb550a27610ecbbeac7c)

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)
Improve documentation

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:

[Big cleanup char to auto_char](https://github.com/profanity-im/profanity/pull/1858/commits/5207b275e180dcd194aea813540396f90c965b51)

Replace `gchar` and `g_free` to `auto_gchar`
Improve readability
Improve documentation for certain functions to increase transparency
Correct certain  `char` functions/variables to `gchar`
Fix most of the cases for 1819 issue

I think you need a commit

to change char to auto_char
fix memleaks (if unrelated to the other commit)
change jid to auto_jid
improve documentation
change g_strfreev() to auto_gcharv

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.

jubalh · 2023-07-05T09:31:47Z

For example in #1863 you now have only one commit. But you do 2 things there:

Improve the documentation and change the variable name of a function
Change from char to auto_char to automatically free it.

These things are not related and should be separate commits.

jubalh · 2023-07-12T11:53:57Z

Your other PR got merged. First commit can be dropped I guess.
Then the rest cleaned up. You dont need a new PR for this. I will review here. It shouldnt be too many lines left after the chat->autochar change.

jubalh

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.

jubalh · 2023-07-12T14:04:48Z

src/config/files.c

- *
- * @note The returned value must be freed using g_free when it is no longer needed
- *       to prevent memory leaks.
- */


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)

Fix 11 potential mem leaks in theme.c

Remove unused variables Apply minor cleanups

Include some additional minor cleanups

jubalh

Looks good! But there are still function description comments in one .h file :)

H3rnand3zzz · 2023-07-13T19:18:51Z

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

jubalh

Huge PR to review. I hope I didn't miss anything. LGTM.

jubalh · 2023-07-14T05:16:10Z

src/config/preferences.c

 void
-prefs_set_string(preference_t pref, char* value)
+prefs_set_string(preference_t pref, gchar* new_value)


I think value was quite descriptive ;)

jubalh · 2023-07-14T05:16:46Z

src/config/preferences.c

 gchar*
-prefs_get_string_with_option(preference_t pref, gchar* option)


It looks like we even don't use this function anymore?

jubalh · 2023-07-14T05:23:09Z

it was for macros, since they are used only in .h file

I see! My bad!

H3rnand3zzz force-pushed the cleanup/auto_char branch 8 times, most recently from cd6d6e2 to 3e6c723 Compare June 16, 2023 11:03

H3rnand3zzz marked this pull request as draft June 16, 2023 11:10

H3rnand3zzz force-pushed the cleanup/auto_char branch 2 times, most recently from f0888fe to e233c5f Compare June 21, 2023 21:40

H3rnand3zzz changed the title ~~auto_char cleanup~~ Major cleanup Jun 23, 2023

H3rnand3zzz force-pushed the cleanup/auto_char branch 3 times, most recently from f5f27ae to 5ed3c3d Compare June 23, 2023 23:34

jubalh assigned H3rnand3zzz Jul 2, 2023

jubalh added the cleanup label Jul 2, 2023

jubalh added this to the next milestone Jul 2, 2023

H3rnand3zzz changed the title ~~Major cleanup~~ Major cleanup (to split) Jul 4, 2023

H3rnand3zzz force-pushed the cleanup/auto_char branch from f30b935 to 8c8419c Compare July 4, 2023 14:06

H3rnand3zzz mentioned this pull request Jul 4, 2023

Change char->free to auto_char char for autocleanup #1863

Merged

2 tasks

H3rnand3zzz force-pushed the cleanup/auto_char branch 2 times, most recently from f30b935 to 830d152 Compare July 7, 2023 06:47

H3rnand3zzz force-pushed the cleanup/auto_char branch from 830d152 to 946eee6 Compare July 12, 2023 13:37

H3rnand3zzz marked this pull request as ready for review July 12, 2023 13:39

H3rnand3zzz changed the title ~~Major cleanup (to split)~~ Major cleanup Jul 12, 2023

jubalh self-requested a review July 12, 2023 14:07

jubalh requested changes Jul 12, 2023

View reviewed changes

Cleanup and mem leak fix

3a4cd7d

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)

H3rnand3zzz force-pushed the cleanup/auto_char branch from 946eee6 to 69bb2d8 Compare July 13, 2023 14:46

H3rnand3zzz added 3 commits July 13, 2023 17:04

g_free() to auto_gfree, introduce auto_guchar

8304ac8

Fix 11 potential mem leaks in theme.c

Cleanup jid_destroy to auto_jid

029f1ca

Remove unused variables Apply minor cleanups

Cleanup g_strfreev() to auto_gcharv

865a056

Include some additional minor cleanups

H3rnand3zzz force-pushed the cleanup/auto_char branch 2 times, most recently from 8861cb0 to a52995d Compare July 13, 2023 17:00

jubalh requested changes Jul 13, 2023

View reviewed changes

Improve documentation

09f217d

H3rnand3zzz force-pushed the cleanup/auto_char branch from a52995d to 09f217d Compare July 13, 2023 19:22

jubalh self-requested a review July 14, 2023 05:22

jubalh approved these changes Jul 14, 2023

View reviewed changes

jubalh merged commit 4814887 into profanity-im:master Jul 14, 2023
6 checks passed

H3rnand3zzz deleted the cleanup/auto_char branch August 22, 2023 11:49

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Major cleanup #1858

Major cleanup #1858

H3rnand3zzz commented Jun 15, 2023 •

edited

Loading

sjaeckel commented Jun 29, 2023

H3rnand3zzz commented Jul 4, 2023

jubalh commented Jul 5, 2023

jubalh commented Jul 5, 2023

jubalh commented Jul 12, 2023

jubalh left a comment •

edited

Loading

jubalh Jul 12, 2023

jubalh left a comment

H3rnand3zzz commented Jul 13, 2023

jubalh left a comment

jubalh Jul 14, 2023

jubalh Jul 14, 2023

jubalh commented Jul 14, 2023

		gchar*
		prefs_get_string_with_option(preference_t pref, gchar* option)

Major cleanup #1858

Major cleanup #1858

Conversation

H3rnand3zzz commented Jun 15, 2023 • edited Loading

sjaeckel commented Jun 29, 2023

H3rnand3zzz commented Jul 4, 2023

jubalh commented Jul 5, 2023

jubalh commented Jul 5, 2023

jubalh commented Jul 12, 2023

jubalh left a comment • edited Loading

Choose a reason for hiding this comment

jubalh Jul 12, 2023

Choose a reason for hiding this comment

jubalh left a comment

Choose a reason for hiding this comment

H3rnand3zzz commented Jul 13, 2023

jubalh left a comment

Choose a reason for hiding this comment

jubalh Jul 14, 2023

Choose a reason for hiding this comment

jubalh Jul 14, 2023

Choose a reason for hiding this comment

jubalh commented Jul 14, 2023

H3rnand3zzz commented Jun 15, 2023 •

edited

Loading

jubalh left a comment •

edited

Loading