Summary

Create the PurpleTags object for handling tags

Review Request #1777 — Created Sept. 17, 2022 and submitted Sept. 23, 2022, 5:36 a.m.

Information

Owner

grim

Repository

pidgin/pidgin

Branch

default

Bugs

PIDGIN-17664

Depends On

Reviewers

Groups

pidgin

People

Description

Create the PurpleTags object for handling tags

Testing Done

Ran the unit tests.

Commits

Summary	ID
Create the PurpleTags object for handling tags	eda5179fe1aabcb635b4b007fc46c37ec3a9f8a1

Issues

Description	From	Last Updated
Build warning on this line: Warning: Purple: purple_tags_get_all: return value: Missing (element-type) annotation	ivanhoe	Sept. 23, 2022, 4:18 a.m.
Build warning on this line: ../libpurple/purpletags.h:139: Warning: Purple: missing ":" at column 26: * @separator: (nullable) A string to separate …	ivanhoe	Sept. 23, 2022, 4:18 a.m.
Double 'the'. Also 'value of the first tag'	QuLogic	Sept. 23, 2022, 4:05 a.m.
Align wrapped text.	QuLogic	Sept. 23, 2022, 5:12 a.m.
Extra space.	QuLogic	Sept. 23, 2022, 4:05 a.m.
Do we want a gboolean *found here, to avoid two loops? Or maybe add another function named purple_tags_lookup which returns …	QuLogic	Sept. 23, 2022, 4:06 a.m.
Might also want to check for foo lookup with a foobar tag, as you noticed it was (temporarily) broken on …	QuLogic	Sept. 23, 2022, 5:12 a.m.
existent	QuLogic	Sept. 23, 2022, 4:04 a.m.
existent	QuLogic	Sept. 23, 2022, 4:04 a.m.
Should be g_assert_cmpstr.	QuLogic	Sept. 23, 2022, 4:04 a.m.
Should be g_assert_cmpstr.	QuLogic	Sept. 23, 2022, 4:04 a.m.
Should be g_assert_cmpstr.	QuLogic	Sept. 23, 2022, 4:04 a.m.
This loop should also check that you didn't get more or less tags than values.	QuLogic	Sept. 23, 2022, 5:13 a.m.
Currently only checking that there are not more tags than values, but not vice versa.	QuLogic	Sept. 23, 2022, 5:13 a.m.
This comparison is off-by-one as you have NULL in values, but that's not added as a tag.	QuLogic	Sept. 23, 2022, 5:13 a.m.
Should be g_assert_cmpstr.	QuLogic	Sept. 23, 2022, 4:04 a.m.
Should be g_assert_cmpstr.	QuLogic	Sept. 23, 2022, 4:04 a.m.
Should be g_assert_cmpstr.	QuLogic	Sept. 23, 2022, 4:04 a.m.

When running the unit tests I get some "still reachable" memory in valgrind. Not sure if that's a real issue.

grim Sept. 22, 2022, 10:36 p.m.

How'd you run it? Because I'm not seeing any leaks...

$ valgrind --leak-check=full --suppressions=/usr/share/glib-2.0/valgrind/glib.supp ./libpurple/tests/test_tags
==1983340== Memcheck, a memory error detector
==1983340== Copyright (C) 2002-2022, and GNU GPL'd, by Julian Seward et al.
==1983340== Using Valgrind-3.19.0 and LibVEX; rerun with -h for copyright info
==1983340== Command: ./libpurple/tests/test_tags
==1983340== 
# random seed: R02Safa67fe99044fa42a9ed721a3a05fc8f
1..14
# Start of tags tests
ok 1 /tags/exists-true
ok 2 /tags/exists-false
ok 3 /tags/add-remove-bare
ok 4 /tags/add-duplicate-bare
ok 5 /tags/remove-non-existant-bare
ok 6 /tags/add-remove-with-value
ok 7 /tags/add-duplicate-with-value
ok 8 /tags/remove-non-existant-with-value
ok 9 /tags/get-single
ok 10 /tags/get-multiple
ok 11 /tags/get-all
ok 12 /tags/to-string-single
ok 13 /tags/to-string-multiple-with-separator
ok 14 /tags/to-string-multiple-with-null-separator
# End of tags tests
==1983340== 
==1983340== HEAP SUMMARY:
==1983340==     in use at exit: 47,614 bytes in 286 blocks
==1983340==   total heap usage: 1,090 allocs, 804 frees, 1,311,391 bytes allocated
==1983340== 
==1983340== LEAK SUMMARY:
==1983340==    definitely lost: 0 bytes in 0 blocks
==1983340==    indirectly lost: 0 bytes in 0 blocks
==1983340==      possibly lost: 0 bytes in 0 blocks
==1983340==    still reachable: 2,400 bytes in 31 blocks
==1983340==         suppressed: 43,118 bytes in 233 blocks
==1983340== Reachable blocks (those to which a pointer was found) are not shown.
==1983340== To see them, rerun with: --leak-check=full --show-leak-kinds=all
==1983340== 
==1983340== For lists of detected and suppressed errors, rerun with: -s
==1983340== ERROR SUMMARY: 0 errors from 0 contexts (suppressed: 0 from 0)

ivanhoe Sept. 23, 2022, 4:16 a.m.

With --show-leak-kinds=all and without the glib suppressions. There are some "still reachables" originating from purple_tags_new which I though might be an issue. For example:
==20999== 96 bytes in 1 blocks are still reachable in loss record 243 of 263
==20999==    at 0x48447E5: malloc (vg_replace_malloc.c:381)
==20999==    by 0x4A37E68: g_malloc (in /usr/lib64/libglib-2.0.so.0.7200.3)
==20999==    by 0x49A914B: g_signal_newv (in /usr/lib64/libgobject-2.0.so.0.7200.3)
==20999==    by 0x49A96C2: g_signal_new_valist (in /usr/lib64/libgobject-2.0.so.0.7200.3)
==20999==    by 0x49A9828: g_signal_new (in /usr/lib64/libgobject-2.0.so.0.7200.3)
==20999==    by 0x4999898: g_object_do_class_init (in /usr/lib64/libgobject-2.0.so.0.7200.3)
==20999==    by 0x49B3B57: g_type_class_ref (in /usr/lib64/libgobject-2.0.so.0.7200.3)
==20999==    by 0x49B3834: g_type_class_ref (in /usr/lib64/libgobject-2.0.so.0.7200.3)
==20999==    by 0x499C1E7: g_object_new_with_properties (in /usr/lib64/libgobject-2.0.so.0.7200.3)
==20999==    by 0x499CB78: g_object_new (in /usr/lib64/libgobject-2.0.so.0.7200.3)
==20999==    by 0x48FA090: purple_tags_new (purpletags.c:67)
==20999==    by 0x10927D: test_purple_tags_exists_true (test_tags.c:32)

grim Sept. 23, 2022, 5 a.m.

Can't reproduce with the glib suppressions turned on.

libpurple/purpletags.h (Diff revision 1)
```
I think then should be the here.
```

libpurple/purpletags.h (Diff revision 1)

Show all issues

Build warning on this line:

Warning: Purple: purple_tags_get_all: return value: Missing (element-type) annotation

libpurple/purpletags.h (Diff revision 1)

Show all issues

Build warning on this line:

../libpurple/purpletags.h:139: Warning: Purple: missing ":" at column 26:
 * @separator: (nullable) A string to separate the items.
                         ^

Change Summary:

Address issues

Commits:

	Summary	ID
	Create the PurpleTags object for handling tags	2c6464e0b9c135ea88cf4c285e754a37faa8fd9d
	Create the PurpleTags object for handling tags	5f33ed32475d01e5c9de49bdd7d1dede36059b78

Diff:

Revision 2 (+1188)

Show changes

	libpurple/meson.build
	libpurple/purpletags.h
	libpurple/purpletags.c
	libpurple/tests/meson.build
	libpurple/tests/test_tags.c

libpurple/purpletags.h (Diff revision 2)
Show all issues
```
Double 'the'. Also 'value of the first tag'
```
libpurple/purpletags.c (Diff revision 2)
Show all issues
```
Extra space.
```

libpurple/purpletags.c (Diff revision 2)

Show all issues

Do we want a gboolean *found here, to avoid two loops?
Or maybe add another function named purple_tags_lookup which returns the value and whether it was found?

grim Sept. 23, 2022, 3:23 a.m.

yeah lets replace purple_tags_exists with purple_tags_lookup with the out parameter.

libpurple/tests/test_tags.c (Diff revision 2)
Show all issues
```
existent
```
libpurple/tests/test_tags.c (Diff revision 2)
Show all issues
```
existent
```
libpurple/tests/test_tags.c (Diff revision 2)
Show all issues
```
Should be g_assert_cmpstr.
```
libpurple/tests/test_tags.c (Diff revision 2)
Show all issues
```
Should be g_assert_cmpstr.
```
libpurple/tests/test_tags.c (Diff revision 2)
Show all issues
```
Should be g_assert_cmpstr.
```

libpurple/tests/test_tags.c (Diff revision 2)

Show all issues

This loop should also check that you didn't get more or less tags than values.

libpurple/tests/test_tags.c (Diff revision 2)
Show all issues
```
Should be g_assert_cmpstr.
```
libpurple/tests/test_tags.c (Diff revision 2)
Show all issues
```
Should be g_assert_cmpstr.
```
libpurple/tests/test_tags.c (Diff revision 2)
Show all issues
```
Should be g_assert_cmpstr.
```

Change Summary:

address issues, replace purple_tags_exist with purple_tags_lookup, have purple_tags_get use purple_tags_lookup.

Commits:

	Summary	ID
	Create the PurpleTags object for handling tags	5f33ed32475d01e5c9de49bdd7d1dede36059b78
	Create the PurpleTags object for handling tags	ca8ce78609bf9507e17e6cb9f4ce40a64810f961

Diff:

Revision 3 (+1224)

Show changes

	libpurple/meson.build
	libpurple/purpletags.h
	libpurple/purpletags.c
	libpurple/tests/meson.build
	libpurple/tests/test_tags.c

libpurple/purpletags.h (Diff revisions 2 - 3)
Show all issues
```
Align wrapped text.
```

libpurple/tests/test_tags.c (Diff revisions 2 - 3)

Show all issues

Might also want to check for foo lookup with a foobar tag, as you noticed it was (temporarily) broken on stream.

libpurple/tests/test_tags.c (Diff revisions 2 - 3)

Show all issues

Currently only checking that there are not more tags than values, but not vice versa.

libpurple/tests/test_tags.c (Diff revisions 2 - 3)

Show all issues

This comparison is off-by-one as you have NULL in values, but that's not added as a tag.

Change Summary:

Address issues (again).

Commits:

	Summary	ID
	Create the PurpleTags object for handling tags	ca8ce78609bf9507e17e6cb9f4ce40a64810f961
	Create the PurpleTags object for handling tags	eda5179fe1aabcb635b4b007fc46c37ec3a9f8a1

Diff:

Revision 4 (+1244)

Show changes

	libpurple/meson.build
	libpurple/purpletags.h
	libpurple/purpletags.c
	libpurple/tests/meson.build
	libpurple/tests/test_tags.c

Ship it!

```
Ship It!
```

Review Board 6.0.2

Create the PurpleTags object for handling tags

Information

Reviewers

Change Summary:

Commits:

Diff:

Change Summary:

Commits:

Diff:

Change Summary:

Commits:

Diff:

Status: Closed (submitted)