[PATCH 0/3] ruby: get rid of Tags object

[PATCH 0/3] ruby: get rid of Tags object

[Felipe Contreras]

We don't need a Tags enumerable object only for a small number of strings, we
can just get them directly.

This fixes an interaction problem where we might request two tags iterables
from the same message:

  tags_0 = notmuch_message_get_tags(message);
  // Store it for later
  tags_1 = notmuch_message_get_tags(message);
  // Traverse it

This iterator is meant to be transient and works only once, so we better just
iterate it once.

Felipe Contreras (3):
  ruby: add tags helper
  ruby: tags: return string array directly
  ruby: remove Tags object

 bindings/ruby/database.c |  2 +-
 bindings/ruby/defs.h     |  6 +----
 bindings/ruby/init.c     | 14 -----------
 bindings/ruby/message.c  |  2 +-
 bindings/ruby/messages.c |  2 +-
 bindings/ruby/tags.c     | 54 ++++------------------------------------
 bindings/ruby/thread.c   |  2 +-
 7 files changed, 10 insertions(+), 72 deletions(-)

-- 
2.39.2.13.g1fb56cf030

[PATCH 1/3] ruby: add tags helper

[Felipe Contreras]

Right now it doesn't do much, but it will help for further
reorganization.

Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
---
 bindings/ruby/database.c | 2 +-
 bindings/ruby/defs.h     | 3 +++
 bindings/ruby/message.c  | 2 +-
 bindings/ruby/messages.c | 2 +-
 bindings/ruby/tags.c     | 6 ++++++
 bindings/ruby/thread.c   | 2 +-
 6 files changed, 13 insertions(+), 4 deletions(-)

diff --git a/bindings/ruby/database.c b/bindings/ruby/database.c
index 9c3dbd96..9cac6005 100644
--- a/bindings/ruby/database.c
+++ b/bindings/ruby/database.c
@@ -408,7 +408,7 @@ notmuch_rb_database_get_all_tags (VALUE self)
 
 	rb_raise (notmuch_rb_eBaseError, "%s", msg);
     }
-    return Data_Wrap_Notmuch_Object (notmuch_rb_cTags, &notmuch_rb_tags_type, tags);
+    return notmuch_rb_tags_get (tags);
 }
 
 /*
diff --git a/bindings/ruby/defs.h b/bindings/ruby/defs.h
index e2541e8f..9454658b 100644
--- a/bindings/ruby/defs.h
+++ b/bindings/ruby/defs.h
@@ -369,6 +369,9 @@ VALUE
 notmuch_rb_message_thaw (VALUE self);
 
 /* tags.c */
+VALUE
+notmuch_rb_tags_get (notmuch_tags_t *tags);
+
 VALUE
 notmuch_rb_tags_destroy (VALUE self);
 
diff --git a/bindings/ruby/message.c b/bindings/ruby/message.c
index f45c95cc..81085f75 100644
--- a/bindings/ruby/message.c
+++ b/bindings/ruby/message.c
@@ -221,7 +221,7 @@ notmuch_rb_message_get_tags (VALUE self)
     if (!tags)
 	rb_raise (notmuch_rb_eMemoryError, "Out of memory");
 
-    return Data_Wrap_Notmuch_Object (notmuch_rb_cTags, &notmuch_rb_tags_type, tags);
+    return notmuch_rb_tags_get (tags);
 }
 
 /*
diff --git a/bindings/ruby/messages.c b/bindings/ruby/messages.c
index ca5b10d0..6369d052 100644
--- a/bindings/ruby/messages.c
+++ b/bindings/ruby/messages.c
@@ -71,5 +71,5 @@ notmuch_rb_messages_collect_tags (VALUE self)
     if (!tags)
 	rb_raise (notmuch_rb_eMemoryError, "Out of memory");
 
-    return Data_Wrap_Notmuch_Object (notmuch_rb_cTags, &notmuch_rb_tags_type, tags);
+    return notmuch_rb_tags_get (tags);
 }
diff --git a/bindings/ruby/tags.c b/bindings/ruby/tags.c
index 2af85e36..cc6ea59e 100644
--- a/bindings/ruby/tags.c
+++ b/bindings/ruby/tags.c
@@ -20,6 +20,12 @@
 
 #include "defs.h"
 
+VALUE
+notmuch_rb_tags_get (notmuch_tags_t *tags)
+{
+    return Data_Wrap_Notmuch_Object (notmuch_rb_cTags, &notmuch_rb_tags_type, tags);
+}
+
 /*
  * call-seq: TAGS.destroy! => nil
  *
diff --git a/bindings/ruby/thread.c b/bindings/ruby/thread.c
index 7cb2a3dc..b20ed893 100644
--- a/bindings/ruby/thread.c
+++ b/bindings/ruby/thread.c
@@ -204,5 +204,5 @@ notmuch_rb_thread_get_tags (VALUE self)
     if (!tags)
 	rb_raise (notmuch_rb_eMemoryError, "Out of memory");
 
-    return Data_Wrap_Notmuch_Object (notmuch_rb_cTags, &notmuch_rb_tags_type, tags);
+    return notmuch_rb_tags_get (tags);
 }
-- 
2.39.2.13.g1fb56cf030

[PATCH 2/3] ruby: tags: return string array directly

[Felipe Contreras]

Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
---
 bindings/ruby/tags.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/bindings/ruby/tags.c b/bindings/ruby/tags.c
index cc6ea59e..cad17d4c 100644
--- a/bindings/ruby/tags.c
+++ b/bindings/ruby/tags.c
@@ -23,7 +23,12 @@
 VALUE
 notmuch_rb_tags_get (notmuch_tags_t *tags)
 {
-    return Data_Wrap_Notmuch_Object (notmuch_rb_cTags, &notmuch_rb_tags_type, tags);
+    VALUE rb_array = rb_ary_new();
+    for (; notmuch_tags_valid (tags); notmuch_tags_move_to_next (tags)) {
+	const char *tag = notmuch_tags_get (tags);
+	rb_ary_push (rb_array, rb_str_new2 (tag));
+    }
+    return rb_array;
 }
 
 /*
-- 
2.39.2.13.g1fb56cf030

[PATCH 3/3] ruby: remove Tags object

[Felipe Contreras]

Not used anymore now that we return an array of strings directly.

Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
---
 bindings/ruby/defs.h |  7 ------
 bindings/ruby/init.c | 14 -----------
 bindings/ruby/tags.c | 55 --------------------------------------------
 3 files changed, 76 deletions(-)

diff --git a/bindings/ruby/defs.h b/bindings/ruby/defs.h
index 9454658b..027408a1 100644
--- a/bindings/ruby/defs.h
+++ b/bindings/ruby/defs.h
@@ -33,7 +33,6 @@ extern VALUE notmuch_rb_cThreads;
 extern VALUE notmuch_rb_cThread;
 extern VALUE notmuch_rb_cMessages;
 extern VALUE notmuch_rb_cMessage;
-extern VALUE notmuch_rb_cTags;
 
 extern VALUE notmuch_rb_eBaseError;
 extern VALUE notmuch_rb_eDatabaseError;
@@ -372,12 +371,6 @@ notmuch_rb_message_thaw (VALUE self);
 VALUE
 notmuch_rb_tags_get (notmuch_tags_t *tags);
 
-VALUE
-notmuch_rb_tags_destroy (VALUE self);
-
-VALUE
-notmuch_rb_tags_each (VALUE self);
-
 /* init.c */
 void
 Init_notmuch (void);
diff --git a/bindings/ruby/init.c b/bindings/ruby/init.c
index cd9f04cd..db6e7e5a 100644
--- a/bindings/ruby/init.c
+++ b/bindings/ruby/init.c
@@ -28,7 +28,6 @@ VALUE notmuch_rb_cThreads;
 VALUE notmuch_rb_cThread;
 VALUE notmuch_rb_cMessages;
 VALUE notmuch_rb_cMessage;
-VALUE notmuch_rb_cTags;
 
 VALUE notmuch_rb_eBaseError;
 VALUE notmuch_rb_eDatabaseError;
@@ -71,7 +70,6 @@ define_type (threads);
 define_type (thread);
 define_type (messages);
 define_type (message);
-define_type (tags);
 
 /*
  * Document-module: Notmuch
@@ -92,7 +90,6 @@ define_type (tags);
  * - Notmuch::Messages
  * - Notmuch::Thread
  * - Notmuch::Message
- * - Notmuch::Tags
  */
 
 void
@@ -395,15 +392,4 @@ Init_notmuch (void)
     rb_define_method (notmuch_rb_cMessage, "tags_to_maildir_flags", notmuch_rb_message_tags_to_maildir_flags, 0); /* in message.c */
     rb_define_method (notmuch_rb_cMessage, "freeze", notmuch_rb_message_freeze, 0); /* in message.c */
     rb_define_method (notmuch_rb_cMessage, "thaw", notmuch_rb_message_thaw, 0); /* in message.c */
-
-    /*
-     * Document-class: Notmuch::Tags
-     *
-     * Notmuch tags
-     */
-    notmuch_rb_cTags = rb_define_class_under (mod, "Tags", rb_cObject);
-    rb_undef_method (notmuch_rb_cTags, "initialize");
-    rb_define_method (notmuch_rb_cTags, "destroy!", notmuch_rb_tags_destroy, 0); /* in tags.c */
-    rb_define_method (notmuch_rb_cTags, "each", notmuch_rb_tags_each, 0); /* in tags.c */
-    rb_include_module (notmuch_rb_cTags, rb_mEnumerable);
 }
diff --git a/bindings/ruby/tags.c b/bindings/ruby/tags.c
index cad17d4c..cc5d666d 100644
--- a/bindings/ruby/tags.c
+++ b/bindings/ruby/tags.c
@@ -1,23 +1,3 @@
-/* The Ruby interface to the notmuch mail library
- *
- * Copyright © 2010, 2011 Ali Polatel
- *
- * This program is free software: you can redistribute it and/or modify
- * it under the terms of the GNU General Public License as published by
- * the Free Software Foundation, either version 3 of the License, or
- * (at your option) any later version.
- *
- * This program is distributed in the hope that it will be useful,
- * but WITHOUT ANY WARRANTY; without even the implied warranty of
- * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
- * GNU General Public License for more details.
- *
- * You should have received a copy of the GNU General Public License
- * along with this program.  If not, see https://www.gnu.org/licenses/ .
- *
- * Author: Ali Polatel <alip@exherbo.org>
- */
-
 #include "defs.h"
 
 VALUE
@@ -30,38 +10,3 @@ notmuch_rb_tags_get (notmuch_tags_t *tags)
     }
     return rb_array;
 }
-
-/*
- * call-seq: TAGS.destroy! => nil
- *
- * Destroys the tags, freeing all resources allocated for it.
- */
-VALUE
-notmuch_rb_tags_destroy (VALUE self)
-{
-    notmuch_rb_object_destroy (self, &notmuch_rb_tags_type);
-
-    return Qnil;
-}
-
-/*
- * call-seq: TAGS.each {|item| block } => TAGS
- *
- * Calls +block+ once for each element in +self+, passing that element as a
- * parameter.
- */
-VALUE
-notmuch_rb_tags_each (VALUE self)
-{
-    const char *tag;
-    notmuch_tags_t *tags;
-
-    Data_Get_Notmuch_Tags (self, tags);
-
-    for (; notmuch_tags_valid (tags); notmuch_tags_move_to_next (tags)) {
-	tag = notmuch_tags_get (tags);
-	rb_yield (rb_str_new2 (tag));
-    }
-
-    return self;
-}
-- 
2.39.2.13.g1fb56cf030
\r

Re: [PATCH 0/3] ruby: get rid of Tags object

[David Bremner]

Felipe Contreras <felipe.contreras@gmail.com> writes:

> We don't need a Tags enumerable object only for a small number of strings, we
> can just get them directly.
>
> This fixes an interaction problem where we might request two tags iterables
> from the same message:
>
>   tags_0 = notmuch_message_get_tags(message);

I have applied this series to master.

By the way I noticed that the formatting of the old bindings code does
not match the output of uncrustify -c ./devel/uncrustify/cfg $file.
This is not a serious problem, but it does make the review process a bit
noisy (since my clumsy script reformats every file touched by a given
commit). Do you have any objection to my just going through and
reformatting the bindings code with uncrustify at some point? It can be
done as patches also, but it's a bit silly because there can be many
small diffs.

d

Re: [PATCH 0/3] ruby: get rid of Tags object

[Felipe Contreras]

On Sun, Apr 2, 2023 at 5:18 PM David Bremner <david@tethera.net> wrote:
>
> Felipe Contreras <felipe.contreras@gmail.com> writes:
>
> > We don't need a Tags enumerable object only for a small number of strings, we
> > can just get them directly.
> >
> > This fixes an interaction problem where we might request two tags iterables
> > from the same message:
> >
> >   tags_0 = notmuch_message_get_tags(message);
>
> I have applied this series to master.
>
> By the way I noticed that the formatting of the old bindings code does
> not match the output of uncrustify -c ./devel/uncrustify/cfg $file.
> This is not a serious problem, but it does make the review process a bit
> noisy (since my clumsy script reformats every file touched by a given
> commit). Do you have any objection to my just going through and
> reformatting the bindings code with uncrustify at some point? It can be
> done as patches also, but it's a bit silly because there can be many
> small diffs.

I have no problem with changing the format either in one go or as
separate patches.

*Except* for `sp_not`: `if (! cond)` makes my eyes bleed. For me unary
operators act on the thing next to them, so a space before that makes
it less clear, not more.

Given that I'm the one that has worked on these bindings the most, and
probably will be the one that works on them the most in the future, I
would appreciate that small compromise on the coding style.

Cheers.

-- 
Felipe Contreras\r