Create the PurpleTags object for handling tags

Review Request #1777 — Created Sept. 17, 2022 and submitted

grim
pidgin/pidgin
default
PIDGIN-17664
pidgin
Create the PurpleTags object for handling tags

Ran the unit tests.

Summary
Create the PurpleTags object for handling tags
Description From Last Updated

Build warning on this line: Warning: Purple: purple_tags_get_all: return value: Missing (element-type) annotation

ivanhoeivanhoe

Build warning on this line: ../libpurple/purpletags.h:139: Warning: Purple: missing ":" at column 26: * @separator: (nullable) A string to separate …

ivanhoeivanhoe

Double 'the'. Also 'value of the first tag'

QuLogicQuLogic

Align wrapped text.

QuLogicQuLogic

Extra space.

QuLogicQuLogic

Do we want a gboolean *found here, to avoid two loops? Or maybe add another function named purple_tags_lookup which returns …

QuLogicQuLogic

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

QuLogicQuLogic

existent

QuLogicQuLogic

existent

QuLogicQuLogic

Should be g_assert_cmpstr.

QuLogicQuLogic

Should be g_assert_cmpstr.

QuLogicQuLogic

Should be g_assert_cmpstr.

QuLogicQuLogic

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

QuLogicQuLogic

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

QuLogicQuLogic

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

QuLogicQuLogic

Should be g_assert_cmpstr.

QuLogicQuLogic

Should be g_assert_cmpstr.

QuLogicQuLogic

Should be g_assert_cmpstr.

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

    1. 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)
      
    2. 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)
      
    3. Can't reproduce with the glib suppressions turned on.

  3. libpurple/purpletags.h (Diff revision 1)
     
     

    I think then should be the here.

  4. libpurple/purpletags.h (Diff revision 1)
     
     

    Build warning on this line:
    Warning: Purple: purple_tags_get_all: return value: Missing (element-type) annotation

  5. libpurple/purpletags.h (Diff revision 1)
     
     

    Build warning on this line:

    ../libpurple/purpletags.h:139: Warning: Purple: missing ":" at column 26:
     * @separator: (nullable) A string to separate the items.
                             ^
    
  6. 
      
grim
QuLogic
  1. 
      
  2. libpurple/purpletags.h (Diff revision 2)
     
     

    Double 'the'. Also 'value of the first tag'

  3. libpurple/purpletags.c (Diff revision 2)
     
     

    Extra space.

  4. libpurple/purpletags.c (Diff revision 2)
     
     

    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?

    1. yeah lets replace purple_tags_exists with purple_tags_lookup with the out parameter.

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

    existent

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

    existent

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

    Should be g_assert_cmpstr.

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

    Should be g_assert_cmpstr.

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

    Should be g_assert_cmpstr.

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

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

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

    Should be g_assert_cmpstr.

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

    Should be g_assert_cmpstr.

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

    Should be g_assert_cmpstr.

  14. 
      
grim
QuLogic
  1. 
      
  2. libpurple/purpletags.h (Diff revisions 2 - 3)
     
     

    Align wrapped text.

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

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

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

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

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

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

  6. 
      
grim
QuLogic
  1. Ship It!
  2. 
      
grim
Review request changed

Status: Closed (submitted)

Loading...