[PATCH v2 00/10] ruby: object cleanups

[PATCH v2 00/10] ruby: object cleanups

[Felipe Contreras]

This superseeds my previous series [1] with much more cleanups.

An important new change is the move towards RTypedData, which is way superior to the old RData
objects.

Everything should work basically the same though.

[1] id: 20210503075457.649056-1-felipe.contreras@gmail.com

Felipe Contreras (10):
  ruby: add missing Data_Get_Notmuch helpers
  ruby: improve all Data_Get_Notmuch_* helpers
  ruby: improve general data get helper
  ruby: simplify data get helper
  ruby: fetch class name in case of error
  ruby: add unlikely hint
  ruby: create Data_Wrap_Notmuch_Object helper
  ruby: move towards more modern RTypedData
  ruby: add all data types
  ruby: new notmuch_rb_object_destroy() helper

 bindings/ruby/database.c  |  22 +++-----
 bindings/ruby/defs.h      | 116 ++++++++++++++++++--------------------
 bindings/ruby/directory.c |  11 +---
 bindings/ruby/filenames.c |   7 +--
 bindings/ruby/init.c      |  21 +++++++
 bindings/ruby/message.c   |  13 ++---
 bindings/ruby/messages.c  |  11 +---
 bindings/ruby/query.c     |  11 +---
 bindings/ruby/tags.c      |   7 +--
 bindings/ruby/thread.c    |  13 ++---
 bindings/ruby/threads.c   |   9 +--
 11 files changed, 105 insertions(+), 136 deletions(-)

Range-diff against v1:
 -:  -------- >  1:  9c15fc44 ruby: add missing Data_Get_Notmuch helpers
 1:  6d121221 =  2:  c9d840d3 ruby: improve all Data_Get_Notmuch_* helpers
 2:  1f19091c =  3:  299b2be1 ruby: improve general data get helper
 3:  b5c84295 !  4:  19fa26de ruby: simplify data get helper
    @@ bindings/ruby/defs.h: extern ID ID_db_mode;
     +#define Data_Get_Notmuch_Object(obj, message, ptr)		\
          do {							\
     -	Data_Get_Struct ((obj), type, (ptr));			\
    -+	(ptr) = rb_data_object_get (obj);			\
    ++	(ptr) = rb_data_object_get ((obj));			\
      	if (!(ptr))						\
      	rb_raise (rb_eRuntimeError, (message));			\
          } while (0)
 -:  -------- >  5:  1872c4b5 ruby: fetch class name in case of error
 -:  -------- >  6:  b46bcac8 ruby: add unlikely hint
 -:  -------- >  7:  1bfa0334 ruby: create Data_Wrap_Notmuch_Object helper
 -:  -------- >  8:  a9b7ac45 ruby: move towards more modern RTypedData
 -:  -------- >  9:  e2116d5d ruby: add all data types
 -:  -------- > 10:  f6660e5b ruby: new notmuch_rb_object_destroy() helper
-- 
2.31.0

[PATCH v2 01/10] ruby: add missing Data_Get_Notmuch helpers

[Felipe Contreras]

Apparently commit 5c9e3855 (ruby: Don't barf if an object is destroyed
more than once, 2010-05-26) missed these two.

Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
---
 bindings/ruby/directory.c | 2 +-
 bindings/ruby/threads.c   | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/bindings/ruby/directory.c b/bindings/ruby/directory.c
index 0f37b391..fe5fc46d 100644
--- a/bindings/ruby/directory.c
+++ b/bindings/ruby/directory.c
@@ -30,7 +30,7 @@ notmuch_rb_directory_destroy (VALUE self)
 {
     notmuch_directory_t *dir;
 
-    Data_Get_Struct (self, notmuch_directory_t, dir);
+    Data_Get_Notmuch_Directory (self, dir);
 
     notmuch_directory_destroy (dir);
     DATA_PTR (self) = NULL;
diff --git a/bindings/ruby/threads.c b/bindings/ruby/threads.c
index ed403a8f..5885f565 100644
--- a/bindings/ruby/threads.c
+++ b/bindings/ruby/threads.c
@@ -30,7 +30,7 @@ notmuch_rb_threads_destroy (VALUE self)
 {
     notmuch_threads_t *threads;
 
-    Data_Get_Struct (self, notmuch_threads_t, threads);
+    Data_Get_Notmuch_Threads (self, threads);
 
     notmuch_threads_destroy (threads);
     DATA_PTR (self) = NULL;
-- 
2.31.0

[PATCH v2 02/10] ruby: improve all Data_Get_Notmuch_* helpers

[Felipe Contreras]

There's no need to repeat the same code over and over.

Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
---
 bindings/ruby/defs.h | 81 ++++++++++++--------------------------------
 1 file changed, 22 insertions(+), 59 deletions(-)

diff --git a/bindings/ruby/defs.h b/bindings/ruby/defs.h
index 48544ca2..e95ea239 100644
--- a/bindings/ruby/defs.h
+++ b/bindings/ruby/defs.h
@@ -55,77 +55,40 @@ extern ID ID_db_mode;
 # define RSTRING_PTR(v) (RSTRING((v))->ptr)
 #endif /* !defined (RSTRING_PTR) */
 
-#define Data_Get_Notmuch_Database(obj, ptr)			\
+#define Data_Get_Notmuch_Object(obj, type, message, ptr)	\
     do {							\
 	Check_Type ((obj), T_DATA);				\
 	if (DATA_PTR ((obj)) == NULL)				\
-	rb_raise (rb_eRuntimeError, "database closed");		\
-	Data_Get_Struct ((obj), notmuch_database_t, (ptr));	\
+	rb_raise (rb_eRuntimeError, (message));			\
+	Data_Get_Struct ((obj), type, (ptr));			\
     } while (0)
 
-#define Data_Get_Notmuch_Directory(obj, ptr)			\
-    do {							\
-	Check_Type ((obj), T_DATA);				\
-	if (DATA_PTR ((obj)) == NULL)				\
-	rb_raise (rb_eRuntimeError, "directory destroyed");	\
-	Data_Get_Struct ((obj), notmuch_directory_t, (ptr));	\
-    } while (0)
+#define Data_Get_Notmuch_Database(obj, ptr) \
+    Data_Get_Notmuch_Object ((obj), notmuch_database_t, "database closed", (ptr))
 
-#define Data_Get_Notmuch_FileNames(obj, ptr)			\
-    do {							\
-	Check_Type ((obj), T_DATA);				\
-	if (DATA_PTR ((obj)) == NULL)				\
-	rb_raise (rb_eRuntimeError, "filenames destroyed");	\
-	Data_Get_Struct ((obj), notmuch_filenames_t, (ptr));	\
-    } while (0)
+#define Data_Get_Notmuch_Directory(obj, ptr) \
+    Data_Get_Notmuch_Object ((obj), notmuch_directory_t, "directory destroyed", (ptr))
 
-#define Data_Get_Notmuch_Query(obj, ptr)			\
-    do {							\
-	Check_Type ((obj), T_DATA);				\
-	if (DATA_PTR ((obj)) == NULL)				\
-	rb_raise (rb_eRuntimeError, "query destroyed");		\
-	Data_Get_Struct ((obj), notmuch_query_t, (ptr));	\
-    } while (0)
+#define Data_Get_Notmuch_FileNames(obj, ptr) \
+    Data_Get_Notmuch_Object ((obj), notmuch_filenames_t, "filenames destroyed", (ptr))
 
-#define Data_Get_Notmuch_Threads(obj, ptr)			\
-    do {							\
-	Check_Type ((obj), T_DATA);				\
-	if (DATA_PTR ((obj)) == NULL)				\
-	rb_raise (rb_eRuntimeError, "threads destroyed");	\
-	Data_Get_Struct ((obj), notmuch_threads_t, (ptr));	\
-    } while (0)
+#define Data_Get_Notmuch_Query(obj, ptr) \
+    Data_Get_Notmuch_Object ((obj), notmuch_query_t, "query destroyed", (ptr))
 
-#define Data_Get_Notmuch_Messages(obj, ptr)			\
-    do {							\
-	Check_Type ((obj), T_DATA);				\
-	if (DATA_PTR ((obj)) == NULL)				\
-	rb_raise (rb_eRuntimeError, "messages destroyed");	\
-	Data_Get_Struct ((obj), notmuch_messages_t, (ptr));	\
-    } while (0)
+#define Data_Get_Notmuch_Threads(obj, ptr) \
+    Data_Get_Notmuch_Object ((obj), notmuch_threads_t, "threads destroyed", (ptr))
 
-#define Data_Get_Notmuch_Thread(obj, ptr)			\
-    do {							\
-	Check_Type ((obj), T_DATA);				\
-	if (DATA_PTR ((obj)) == NULL)				\
-	rb_raise (rb_eRuntimeError, "thread destroyed");	\
-	Data_Get_Struct ((obj), notmuch_thread_t, (ptr));	\
-    } while (0)
+#define Data_Get_Notmuch_Messages(obj, ptr) \
+    Data_Get_Notmuch_Object ((obj), notmuch_messages_t, "messages destroyed", (ptr))
 
-#define Data_Get_Notmuch_Message(obj, ptr)			\
-    do {							\
-	Check_Type ((obj), T_DATA);				\
-	if (DATA_PTR ((obj)) == NULL)				\
-	rb_raise (rb_eRuntimeError, "message destroyed");	\
-	Data_Get_Struct ((obj), notmuch_message_t, (ptr));	\
-    } while (0)
+#define Data_Get_Notmuch_Thread(obj, ptr) \
+    Data_Get_Notmuch_Object ((obj), notmuch_thread_t, "thread destroyed", (ptr))
 
-#define Data_Get_Notmuch_Tags(obj, ptr)			\
-    do {						\
-	Check_Type ((obj), T_DATA);			\
-	if (DATA_PTR ((obj)) == NULL)			\
-	rb_raise (rb_eRuntimeError, "tags destroyed");	\
-	Data_Get_Struct ((obj), notmuch_tags_t, (ptr));	\
-    } while (0)
+#define Data_Get_Notmuch_Message(obj, ptr) \
+    Data_Get_Notmuch_Object ((obj), notmuch_message_t, "message destroyed", (ptr))
+
+#define Data_Get_Notmuch_Tags(obj, ptr) \
+    Data_Get_Notmuch_Object ((obj), notmuch_tags_t, "tags destroyed", (ptr))
 
 /* status.c */
 void
-- 
2.31.0

[PATCH v2 03/10] ruby: improve general data get helper

[Felipe Contreras]

There's no need to do Check_Type, Data_Get_Struct calls
rb_data_object_get(), which already does that.

Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
---
 bindings/ruby/defs.h | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/bindings/ruby/defs.h b/bindings/ruby/defs.h
index e95ea239..46e2caf8 100644
--- a/bindings/ruby/defs.h
+++ b/bindings/ruby/defs.h
@@ -57,10 +57,9 @@ extern ID ID_db_mode;
 
 #define Data_Get_Notmuch_Object(obj, type, message, ptr)	\
     do {							\
-	Check_Type ((obj), T_DATA);				\
-	if (DATA_PTR ((obj)) == NULL)				\
-	rb_raise (rb_eRuntimeError, (message));			\
 	Data_Get_Struct ((obj), type, (ptr));			\
+	if (!(ptr))						\
+	rb_raise (rb_eRuntimeError, (message));			\
     } while (0)
 
 #define Data_Get_Notmuch_Database(obj, ptr) \
-- 
2.31.0

Re: [PATCH v2 03/10] ruby: improve general data get helper

[David Bremner]

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

> There's no need to do Check_Type, Data_Get_Struct calls
> rb_data_object_get(), which already does that.

Applied 1-3 to master.

d

[PATCH v2 04/10] ruby: simplify data get helper

[Felipe Contreras]

The type is not actually needed.

Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
---
 bindings/ruby/defs.h | 22 +++++++++++-----------
 1 file changed, 11 insertions(+), 11 deletions(-)

diff --git a/bindings/ruby/defs.h b/bindings/ruby/defs.h
index 46e2caf8..8fb47b4c 100644
--- a/bindings/ruby/defs.h
+++ b/bindings/ruby/defs.h
@@ -55,39 +55,39 @@ extern ID ID_db_mode;
 # define RSTRING_PTR(v) (RSTRING((v))->ptr)
 #endif /* !defined (RSTRING_PTR) */
 
-#define Data_Get_Notmuch_Object(obj, type, message, ptr)	\
+#define Data_Get_Notmuch_Object(obj, message, ptr)		\
     do {							\
-	Data_Get_Struct ((obj), type, (ptr));			\
+	(ptr) = rb_data_object_get ((obj));			\
 	if (!(ptr))						\
 	rb_raise (rb_eRuntimeError, (message));			\
     } while (0)
 
 #define Data_Get_Notmuch_Database(obj, ptr) \
-    Data_Get_Notmuch_Object ((obj), notmuch_database_t, "database closed", (ptr))
+    Data_Get_Notmuch_Object ((obj), "database closed", (ptr))
 
 #define Data_Get_Notmuch_Directory(obj, ptr) \
-    Data_Get_Notmuch_Object ((obj), notmuch_directory_t, "directory destroyed", (ptr))
+    Data_Get_Notmuch_Object ((obj), "directory destroyed", (ptr))
 
 #define Data_Get_Notmuch_FileNames(obj, ptr) \
-    Data_Get_Notmuch_Object ((obj), notmuch_filenames_t, "filenames destroyed", (ptr))
+    Data_Get_Notmuch_Object ((obj), "filenames destroyed", (ptr))
 
 #define Data_Get_Notmuch_Query(obj, ptr) \
-    Data_Get_Notmuch_Object ((obj), notmuch_query_t, "query destroyed", (ptr))
+    Data_Get_Notmuch_Object ((obj), "query destroyed", (ptr))
 
 #define Data_Get_Notmuch_Threads(obj, ptr) \
-    Data_Get_Notmuch_Object ((obj), notmuch_threads_t, "threads destroyed", (ptr))
+    Data_Get_Notmuch_Object ((obj), "threads destroyed", (ptr))
 
 #define Data_Get_Notmuch_Messages(obj, ptr) \
-    Data_Get_Notmuch_Object ((obj), notmuch_messages_t, "messages destroyed", (ptr))
+    Data_Get_Notmuch_Object ((obj), "messages destroyed", (ptr))
 
 #define Data_Get_Notmuch_Thread(obj, ptr) \
-    Data_Get_Notmuch_Object ((obj), notmuch_thread_t, "thread destroyed", (ptr))
+    Data_Get_Notmuch_Object ((obj), "thread destroyed", (ptr))
 
 #define Data_Get_Notmuch_Message(obj, ptr) \
-    Data_Get_Notmuch_Object ((obj), notmuch_message_t, "message destroyed", (ptr))
+    Data_Get_Notmuch_Object ((obj), "message destroyed", (ptr))
 
 #define Data_Get_Notmuch_Tags(obj, ptr) \
-    Data_Get_Notmuch_Object ((obj), notmuch_tags_t, "tags destroyed", (ptr))
+    Data_Get_Notmuch_Object ((obj), "tags destroyed", (ptr))
 
 /* status.c */
 void
-- 
2.31.0

Re: [PATCH v2 04/10] ruby: simplify data get helper

[David Bremner]

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

> The type is not actually needed.
>
> Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
> ---
>  bindings/ruby/defs.h | 22 +++++++++++-----------
>  1 file changed, 11 insertions(+), 11 deletions(-)
>
> diff --git a/bindings/ruby/defs.h b/bindings/ruby/defs.h
> index 46e2caf8..8fb47b4c 100644
> --- a/bindings/ruby/defs.h
> +++ b/bindings/ruby/defs.h
> @@ -55,39 +55,39 @@ extern ID ID_db_mode;
>  # define RSTRING_PTR(v) (RSTRING((v))->ptr)
>  #endif /* !defined (RSTRING_PTR) */
>  
> -#define Data_Get_Notmuch_Object(obj, type, message, ptr)	\
> +#define Data_Get_Notmuch_Object(obj, message, ptr)		\
>      do {							\
> -	Data_Get_Struct ((obj), type, (ptr));			\
> +	(ptr) = rb_data_object_get ((obj));			\

Please explain a bit more in the commit message what's going on
here. Why is it OK to replace Data_Get_Struct with rb_data_object_get?

Re: [PATCH v2 04/10] ruby: simplify data get helper

[Felipe Contreras]

On Wed, May 12, 2021 at 4:59 PM David Bremner <david@tethera.net> wrote:
>
> Felipe Contreras <felipe.contreras@gmail.com> writes:
>
> > The type is not actually needed.
> >
> > Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
> > ---
> >  bindings/ruby/defs.h | 22 +++++++++++-----------
> >  1 file changed, 11 insertions(+), 11 deletions(-)
> >
> > diff --git a/bindings/ruby/defs.h b/bindings/ruby/defs.h
> > index 46e2caf8..8fb47b4c 100644
> > --- a/bindings/ruby/defs.h
> > +++ b/bindings/ruby/defs.h
> > @@ -55,39 +55,39 @@ extern ID ID_db_mode;
> >  # define RSTRING_PTR(v) (RSTRING((v))->ptr)
> >  #endif /* !defined (RSTRING_PTR) */
> >
> > -#define Data_Get_Notmuch_Object(obj, type, message, ptr)     \
> > +#define Data_Get_Notmuch_Object(obj, message, ptr)           \
> >      do {                                                     \
> > -     Data_Get_Struct ((obj), type, (ptr));                   \
> > +     (ptr) = rb_data_object_get ((obj));                     \
>
> Please explain a bit more in the commit message what's going on
> here. Why is it OK to replace Data_Get_Struct with rb_data_object_get?

Data_Get_Struct is nothing but a macro that calls rb_data_object_get
with a cast (unnecessary in C).

#define Data_Get_Struct(obj, type, sval) \
    ((sval) = RBIMPL_CAST((type*)rb_data_object_get(obj)))

Is that explanation sufficient?

-- 
Felipe Contreras

[PATCH v2 05/10] ruby: fetch class name in case of error

[Felipe Contreras]

There is not much point in complicating the code for error messages that
can be easily constructed.

Before:

  database closed (RuntimeError)

After:

  Notmuch::Database object destroyed (RuntimeError)

Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
---
 bindings/ruby/defs.h | 30 ++++++++++++++++--------------
 1 file changed, 16 insertions(+), 14 deletions(-)

diff --git a/bindings/ruby/defs.h b/bindings/ruby/defs.h
index 8fb47b4c..ae3ea101 100644
--- a/bindings/ruby/defs.h
+++ b/bindings/ruby/defs.h
@@ -55,39 +55,41 @@ extern ID ID_db_mode;
 # define RSTRING_PTR(v) (RSTRING((v))->ptr)
 #endif /* !defined (RSTRING_PTR) */
 
-#define Data_Get_Notmuch_Object(obj, message, ptr)		\
-    do {							\
-	(ptr) = rb_data_object_get ((obj));			\
-	if (!(ptr))						\
-	rb_raise (rb_eRuntimeError, (message));			\
+#define Data_Get_Notmuch_Object(obj, ptr)					    \
+    do {									    \
+	(ptr) = rb_data_object_get ((obj));					    \
+	if (!(ptr)) {								    \
+	    VALUE cname = rb_class_name (CLASS_OF ((obj)));			    \
+	    rb_raise (rb_eRuntimeError, "%"PRIsVALUE" object destroyed", cname);    \
+	}									    \
     } while (0)
 
 #define Data_Get_Notmuch_Database(obj, ptr) \
-    Data_Get_Notmuch_Object ((obj), "database closed", (ptr))
+    Data_Get_Notmuch_Object ((obj), (ptr))
 
 #define Data_Get_Notmuch_Directory(obj, ptr) \
-    Data_Get_Notmuch_Object ((obj), "directory destroyed", (ptr))
+    Data_Get_Notmuch_Object ((obj), (ptr))
 
 #define Data_Get_Notmuch_FileNames(obj, ptr) \
-    Data_Get_Notmuch_Object ((obj), "filenames destroyed", (ptr))
+    Data_Get_Notmuch_Object ((obj), (ptr))
 
 #define Data_Get_Notmuch_Query(obj, ptr) \
-    Data_Get_Notmuch_Object ((obj), "query destroyed", (ptr))
+    Data_Get_Notmuch_Object ((obj), (ptr))
 
 #define Data_Get_Notmuch_Threads(obj, ptr) \
-    Data_Get_Notmuch_Object ((obj), "threads destroyed", (ptr))
+    Data_Get_Notmuch_Object ((obj), (ptr))
 
 #define Data_Get_Notmuch_Messages(obj, ptr) \
-    Data_Get_Notmuch_Object ((obj), "messages destroyed", (ptr))
+    Data_Get_Notmuch_Object ((obj), (ptr))
 
 #define Data_Get_Notmuch_Thread(obj, ptr) \
-    Data_Get_Notmuch_Object ((obj), "thread destroyed", (ptr))
+    Data_Get_Notmuch_Object ((obj), (ptr))
 
 #define Data_Get_Notmuch_Message(obj, ptr) \
-    Data_Get_Notmuch_Object ((obj), "message destroyed", (ptr))
+    Data_Get_Notmuch_Object ((obj), (ptr))
 
 #define Data_Get_Notmuch_Tags(obj, ptr) \
-    Data_Get_Notmuch_Object ((obj), "tags destroyed", (ptr))
+    Data_Get_Notmuch_Object ((obj), (ptr))
 
 /* status.c */
 void
-- 
2.31.0

[PATCH v2 06/10] ruby: add unlikely hint

[Felipe Contreras]

The error path is very unlikely.

Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
---
 bindings/ruby/defs.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/bindings/ruby/defs.h b/bindings/ruby/defs.h
index ae3ea101..12538a3a 100644
--- a/bindings/ruby/defs.h
+++ b/bindings/ruby/defs.h
@@ -58,7 +58,7 @@ extern ID ID_db_mode;
 #define Data_Get_Notmuch_Object(obj, ptr)					    \
     do {									    \
 	(ptr) = rb_data_object_get ((obj));					    \
-	if (!(ptr)) {								    \
+	if (RB_UNLIKELY (!(ptr))) {						    \
 	    VALUE cname = rb_class_name (CLASS_OF ((obj)));			    \
 	    rb_raise (rb_eRuntimeError, "%"PRIsVALUE" object destroyed", cname);    \
 	}									    \
-- 
2.31.0

[PATCH v2 07/10] ruby: create Data_Wrap_Notmuch_Object helper

[Felipe Contreras]

This makes the code more maintainable and will help in further patches.

No functional changes.

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

diff --git a/bindings/ruby/database.c b/bindings/ruby/database.c
index 416eb709..b9ad3373 100644
--- a/bindings/ruby/database.c
+++ b/bindings/ruby/database.c
@@ -23,7 +23,7 @@
 VALUE
 notmuch_rb_database_alloc (VALUE klass)
 {
-    return Data_Wrap_Struct (klass, NULL, NULL, NULL);
+    return Data_Wrap_Notmuch_Object (klass, NULL);
 }
 
 /*
@@ -266,7 +266,7 @@ notmuch_rb_database_get_directory (VALUE self, VALUE pathv)
     ret = notmuch_database_get_directory (db, path, &dir);
     notmuch_rb_status_raise (ret);
     if (dir)
-	return Data_Wrap_Struct (notmuch_rb_cDirectory, NULL, NULL, dir);
+	return Data_Wrap_Notmuch_Object (notmuch_rb_cDirectory, dir);
     return Qnil;
 }
 
@@ -293,7 +293,7 @@ notmuch_rb_database_add_message (VALUE self, VALUE pathv)
 
     ret = notmuch_database_index_file (db, path, NULL, &message);
     notmuch_rb_status_raise (ret);
-    return rb_assoc_new (Data_Wrap_Struct (notmuch_rb_cMessage, NULL, NULL, message),
+    return rb_assoc_new (Data_Wrap_Notmuch_Object (notmuch_rb_cMessage, message),
         (ret == NOTMUCH_STATUS_DUPLICATE_MESSAGE_ID) ? Qtrue : Qfalse);
 }
 
@@ -344,7 +344,7 @@ notmuch_rb_database_find_message (VALUE self, VALUE idv)
     notmuch_rb_status_raise (ret);
 
     if (message)
-        return Data_Wrap_Struct (notmuch_rb_cMessage, NULL, NULL, message);
+        return Data_Wrap_Notmuch_Object (notmuch_rb_cMessage, message);
     return Qnil;
 }
 
@@ -370,7 +370,7 @@ notmuch_rb_database_find_message_by_filename (VALUE self, VALUE pathv)
     notmuch_rb_status_raise (ret);
 
     if (message)
-        return Data_Wrap_Struct (notmuch_rb_cMessage, NULL, NULL, message);
+        return Data_Wrap_Notmuch_Object (notmuch_rb_cMessage, message);
     return Qnil;
 }
 
@@ -395,7 +395,7 @@ notmuch_rb_database_get_all_tags (VALUE self)
 
 	rb_raise (notmuch_rb_eBaseError, "%s", msg);
     }
-    return Data_Wrap_Struct (notmuch_rb_cTags, NULL, NULL, tags);
+    return Data_Wrap_Notmuch_Object (notmuch_rb_cTags, tags);
 }
 
 /*
@@ -419,5 +419,5 @@ notmuch_rb_database_query_create (VALUE self, VALUE qstrv)
     if (!query)
         rb_raise (notmuch_rb_eMemoryError, "Out of memory");
 
-    return Data_Wrap_Struct (notmuch_rb_cQuery, NULL, NULL, query);
+    return Data_Wrap_Notmuch_Object (notmuch_rb_cQuery, query);
 }
diff --git a/bindings/ruby/defs.h b/bindings/ruby/defs.h
index 12538a3a..fcf1ea39 100644
--- a/bindings/ruby/defs.h
+++ b/bindings/ruby/defs.h
@@ -64,6 +64,9 @@ extern ID ID_db_mode;
 	}									    \
     } while (0)
 
+#define Data_Wrap_Notmuch_Object(klass, ptr)	\
+    Data_Wrap_Struct ((klass), NULL, NULL, (ptr))
+
 #define Data_Get_Notmuch_Database(obj, ptr) \
     Data_Get_Notmuch_Object ((obj), (ptr))
 
diff --git a/bindings/ruby/directory.c b/bindings/ruby/directory.c
index fe5fc46d..36ef3984 100644
--- a/bindings/ruby/directory.c
+++ b/bindings/ruby/directory.c
@@ -92,7 +92,7 @@ notmuch_rb_directory_get_child_files (VALUE self)
 
     fnames = notmuch_directory_get_child_files (dir);
 
-    return Data_Wrap_Struct (notmuch_rb_cFileNames, NULL, NULL, fnames);
+    return Data_Wrap_Notmuch_Object (notmuch_rb_cFileNames, fnames);
 }
 
 /*
@@ -111,5 +111,5 @@ notmuch_rb_directory_get_child_directories (VALUE self)
 
     fnames = notmuch_directory_get_child_directories (dir);
 
-    return Data_Wrap_Struct (notmuch_rb_cFileNames, NULL, NULL, fnames);
+    return Data_Wrap_Notmuch_Object (notmuch_rb_cFileNames, fnames);
 }
diff --git a/bindings/ruby/message.c b/bindings/ruby/message.c
index 6ea82afa..1990bb97 100644
--- a/bindings/ruby/message.c
+++ b/bindings/ruby/message.c
@@ -89,7 +89,7 @@ notmuch_rb_message_get_replies (VALUE self)
 
     messages = notmuch_message_get_replies (message);
 
-    return Data_Wrap_Struct (notmuch_rb_cMessages, NULL, NULL, messages);
+    return Data_Wrap_Notmuch_Object (notmuch_rb_cMessages, messages);
 }
 
 /*
@@ -125,7 +125,7 @@ notmuch_rb_message_get_filenames (VALUE self)
 
     fnames = notmuch_message_get_filenames (message);
 
-    return Data_Wrap_Struct (notmuch_rb_cFileNames, NULL, NULL, fnames);
+    return Data_Wrap_Notmuch_Object (notmuch_rb_cFileNames, fnames);
 }
 
 /*
@@ -226,7 +226,7 @@ notmuch_rb_message_get_tags (VALUE self)
     if (!tags)
 	rb_raise (notmuch_rb_eMemoryError, "Out of memory");
 
-    return Data_Wrap_Struct (notmuch_rb_cTags, NULL, NULL, tags);
+    return Data_Wrap_Notmuch_Object (notmuch_rb_cTags, tags);
 }
 
 /*
diff --git a/bindings/ruby/messages.c b/bindings/ruby/messages.c
index a337feeb..3d1669ee 100644
--- a/bindings/ruby/messages.c
+++ b/bindings/ruby/messages.c
@@ -53,7 +53,7 @@ notmuch_rb_messages_each (VALUE self)
 
     for (; notmuch_messages_valid (messages); notmuch_messages_move_to_next (messages)) {
 	message = notmuch_messages_get (messages);
-	rb_yield (Data_Wrap_Struct (notmuch_rb_cMessage, NULL, NULL, message));
+	rb_yield (Data_Wrap_Notmuch_Object (notmuch_rb_cMessage, message));
     }
 
     return self;
@@ -76,5 +76,5 @@ notmuch_rb_messages_collect_tags (VALUE self)
     if (!tags)
 	rb_raise (notmuch_rb_eMemoryError, "Out of memory");
 
-    return Data_Wrap_Struct (notmuch_rb_cTags, NULL, NULL, tags);
+    return Data_Wrap_Notmuch_Object (notmuch_rb_cTags, tags);
 }
diff --git a/bindings/ruby/query.c b/bindings/ruby/query.c
index 8b46d700..b0fb4ea7 100644
--- a/bindings/ruby/query.c
+++ b/bindings/ruby/query.c
@@ -142,7 +142,7 @@ notmuch_rb_query_search_threads (VALUE self)
     if (status)
 	notmuch_rb_status_raise (status);
 
-    return Data_Wrap_Struct (notmuch_rb_cThreads, NULL, NULL, threads);
+    return Data_Wrap_Notmuch_Object (notmuch_rb_cThreads, threads);
 }
 
 /*
@@ -163,7 +163,7 @@ notmuch_rb_query_search_messages (VALUE self)
     if (status)
 	notmuch_rb_status_raise (status);
 
-    return Data_Wrap_Struct (notmuch_rb_cMessages, NULL, NULL, messages);
+    return Data_Wrap_Notmuch_Object (notmuch_rb_cMessages, messages);
 }
 
 /*
diff --git a/bindings/ruby/thread.c b/bindings/ruby/thread.c
index 9b295981..e09be147 100644
--- a/bindings/ruby/thread.c
+++ b/bindings/ruby/thread.c
@@ -88,7 +88,7 @@ notmuch_rb_thread_get_toplevel_messages (VALUE self)
     if (!messages)
 	rb_raise (notmuch_rb_eMemoryError, "Out of memory");
 
-    return Data_Wrap_Struct (notmuch_rb_cMessages, NULL, NULL, messages);
+    return Data_Wrap_Notmuch_Object (notmuch_rb_cMessages, messages);
 }
 
 /*
@@ -108,7 +108,7 @@ notmuch_rb_thread_get_messages (VALUE self)
     if (!messages)
 	rb_raise (notmuch_rb_eMemoryError, "Out of memory");
 
-    return Data_Wrap_Struct (notmuch_rb_cMessages, NULL, NULL, messages);
+    return Data_Wrap_Notmuch_Object (notmuch_rb_cMessages, messages);
 }
 
 /*
@@ -209,5 +209,5 @@ notmuch_rb_thread_get_tags (VALUE self)
     if (!tags)
 	rb_raise (notmuch_rb_eMemoryError, "Out of memory");
 
-    return Data_Wrap_Struct (notmuch_rb_cTags, NULL, NULL, tags);
+    return Data_Wrap_Notmuch_Object (notmuch_rb_cTags, tags);
 }
diff --git a/bindings/ruby/threads.c b/bindings/ruby/threads.c
index 5885f565..19e09a7e 100644
--- a/bindings/ruby/threads.c
+++ b/bindings/ruby/threads.c
@@ -53,7 +53,7 @@ notmuch_rb_threads_each (VALUE self)
 
     for (; notmuch_threads_valid (threads); notmuch_threads_move_to_next (threads)) {
 	thread = notmuch_threads_get (threads);
-	rb_yield (Data_Wrap_Struct (notmuch_rb_cThread, NULL, NULL, thread));
+	rb_yield (Data_Wrap_Notmuch_Object (notmuch_rb_cThread, thread));
     }
 
     return self;
-- 
2.31.0

[PATCH v2 08/10] ruby: move towards more modern RTypedData

[Felipe Contreras]

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

diff --git a/bindings/ruby/database.c b/bindings/ruby/database.c
index b9ad3373..bb4273e6 100644
--- a/bindings/ruby/database.c
+++ b/bindings/ruby/database.c
@@ -74,7 +74,7 @@ notmuch_rb_database_initialize (int argc, VALUE *argv, VALUE self)
 	mode = NOTMUCH_DATABASE_MODE_READ_ONLY;
     }
 
-    Check_Type (self, T_DATA);
+    rb_check_typeddata (self, &notmuch_rb_object_type);
     if (create)
 	ret = notmuch_database_create (path, &database);
     else
diff --git a/bindings/ruby/defs.h b/bindings/ruby/defs.h
index fcf1ea39..6dbaa85d 100644
--- a/bindings/ruby/defs.h
+++ b/bindings/ruby/defs.h
@@ -55,9 +55,11 @@ extern ID ID_db_mode;
 # define RSTRING_PTR(v) (RSTRING((v))->ptr)
 #endif /* !defined (RSTRING_PTR) */
 
+extern const rb_data_type_t notmuch_rb_object_type;
+
 #define Data_Get_Notmuch_Object(obj, ptr)					    \
     do {									    \
-	(ptr) = rb_data_object_get ((obj));					    \
+	(ptr) = rb_check_typeddata ((obj), &notmuch_rb_object_type);		    \
 	if (RB_UNLIKELY (!(ptr))) {						    \
 	    VALUE cname = rb_class_name (CLASS_OF ((obj)));			    \
 	    rb_raise (rb_eRuntimeError, "%"PRIsVALUE" object destroyed", cname);    \
@@ -65,7 +67,7 @@ extern ID ID_db_mode;
     } while (0)
 
 #define Data_Wrap_Notmuch_Object(klass, ptr)	\
-    Data_Wrap_Struct ((klass), NULL, NULL, (ptr))
+    TypedData_Wrap_Struct ((klass), &notmuch_rb_object_type, (ptr))
 
 #define Data_Get_Notmuch_Database(obj, ptr) \
     Data_Get_Notmuch_Object ((obj), (ptr))
diff --git a/bindings/ruby/init.c b/bindings/ruby/init.c
index 819fd1e3..f3b2e5b1 100644
--- a/bindings/ruby/init.c
+++ b/bindings/ruby/init.c
@@ -46,6 +46,10 @@ ID ID_call;
 ID ID_db_create;
 ID ID_db_mode;
 
+const rb_data_type_t notmuch_rb_object_type = {
+    .wrap_struct_name = "notmuch_object",
+};
+
 /*
  * Document-module: Notmuch
  *
-- 
2.31.0

Re: [PATCH v2 08/10] ruby: move towards more modern RTypedData

[David Bremner]

Here I would also like a bit more back story in the commit message

Re: [PATCH v2 08/10] ruby: move towards more modern RTypedData

[Felipe Contreras]

On Wed, May 12, 2021 at 5:02 PM David Bremner <david@tethera.net> wrote:
>
> Here I would also like a bit more back story in the commit message

I'm not exactly sure what I need to explain here. RTypedData is the
modern way objects are created.

Data_Wrap_Struct is replaced with TypedData_Wrap_Struct, and the
information is stored in a struct rb_data_type_t, rather than passed
as arguments.

Check_Type is replaced with Check_TypedStruct, which is a wrapper for
rb_check_typeddata (with casts).

-- 
Felipe Contreras

[PATCH v2 09/10] ruby: add all data types

[Felipe Contreras]

Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
---
 bindings/ruby/database.c  | 16 ++++++++--------
 bindings/ruby/defs.h      | 37 +++++++++++++++++++++++--------------
 bindings/ruby/directory.c |  4 ++--
 bindings/ruby/init.c      | 16 ++++++++++++++++
 bindings/ruby/message.c   |  6 +++---
 bindings/ruby/messages.c  |  4 ++--
 bindings/ruby/query.c     |  4 ++--
 bindings/ruby/thread.c    |  6 +++---
 bindings/ruby/threads.c   |  2 +-
 9 files changed, 60 insertions(+), 35 deletions(-)

diff --git a/bindings/ruby/database.c b/bindings/ruby/database.c
index bb4273e6..4ecc8f78 100644
--- a/bindings/ruby/database.c
+++ b/bindings/ruby/database.c
@@ -23,7 +23,7 @@
 VALUE
 notmuch_rb_database_alloc (VALUE klass)
 {
-    return Data_Wrap_Notmuch_Object (klass, NULL);
+    return Data_Wrap_Notmuch_Object (klass, &notmuch_rb_database_type, NULL);
 }
 
 /*
@@ -74,7 +74,7 @@ notmuch_rb_database_initialize (int argc, VALUE *argv, VALUE self)
 	mode = NOTMUCH_DATABASE_MODE_READ_ONLY;
     }
 
-    rb_check_typeddata (self, &notmuch_rb_object_type);
+    rb_check_typeddata (self, &notmuch_rb_database_type);
     if (create)
 	ret = notmuch_database_create (path, &database);
     else
@@ -266,7 +266,7 @@ notmuch_rb_database_get_directory (VALUE self, VALUE pathv)
     ret = notmuch_database_get_directory (db, path, &dir);
     notmuch_rb_status_raise (ret);
     if (dir)
-	return Data_Wrap_Notmuch_Object (notmuch_rb_cDirectory, dir);
+	return Data_Wrap_Notmuch_Object (notmuch_rb_cDirectory, &notmuch_rb_directory_type, dir);
     return Qnil;
 }
 
@@ -293,7 +293,7 @@ notmuch_rb_database_add_message (VALUE self, VALUE pathv)
 
     ret = notmuch_database_index_file (db, path, NULL, &message);
     notmuch_rb_status_raise (ret);
-    return rb_assoc_new (Data_Wrap_Notmuch_Object (notmuch_rb_cMessage, message),
+    return rb_assoc_new (Data_Wrap_Notmuch_Object (notmuch_rb_cMessage, &notmuch_rb_message_type, message),
         (ret == NOTMUCH_STATUS_DUPLICATE_MESSAGE_ID) ? Qtrue : Qfalse);
 }
 
@@ -344,7 +344,7 @@ notmuch_rb_database_find_message (VALUE self, VALUE idv)
     notmuch_rb_status_raise (ret);
 
     if (message)
-        return Data_Wrap_Notmuch_Object (notmuch_rb_cMessage, message);
+	return Data_Wrap_Notmuch_Object (notmuch_rb_cMessage, &notmuch_rb_message_type, message);
     return Qnil;
 }
 
@@ -370,7 +370,7 @@ notmuch_rb_database_find_message_by_filename (VALUE self, VALUE pathv)
     notmuch_rb_status_raise (ret);
 
     if (message)
-        return Data_Wrap_Notmuch_Object (notmuch_rb_cMessage, message);
+	return Data_Wrap_Notmuch_Object (notmuch_rb_cMessage, &notmuch_rb_message_type, message);
     return Qnil;
 }
 
@@ -395,7 +395,7 @@ notmuch_rb_database_get_all_tags (VALUE self)
 
 	rb_raise (notmuch_rb_eBaseError, "%s", msg);
     }
-    return Data_Wrap_Notmuch_Object (notmuch_rb_cTags, tags);
+    return Data_Wrap_Notmuch_Object (notmuch_rb_cTags, &notmuch_rb_tags_type, tags);
 }
 
 /*
@@ -419,5 +419,5 @@ notmuch_rb_database_query_create (VALUE self, VALUE qstrv)
     if (!query)
         rb_raise (notmuch_rb_eMemoryError, "Out of memory");
 
-    return Data_Wrap_Notmuch_Object (notmuch_rb_cQuery, query);
+    return Data_Wrap_Notmuch_Object (notmuch_rb_cQuery, &notmuch_rb_query_type, query);
 }
diff --git a/bindings/ruby/defs.h b/bindings/ruby/defs.h
index 6dbaa85d..fa7b9515 100644
--- a/bindings/ruby/defs.h
+++ b/bindings/ruby/defs.h
@@ -56,45 +56,54 @@ extern ID ID_db_mode;
 #endif /* !defined (RSTRING_PTR) */
 
 extern const rb_data_type_t notmuch_rb_object_type;
-
-#define Data_Get_Notmuch_Object(obj, ptr)					    \
+extern const rb_data_type_t notmuch_rb_database_type;
+extern const rb_data_type_t notmuch_rb_directory_type;
+extern const rb_data_type_t notmuch_rb_filenames_type;
+extern const rb_data_type_t notmuch_rb_query_type;
+extern const rb_data_type_t notmuch_rb_threads_type;
+extern const rb_data_type_t notmuch_rb_thread_type;
+extern const rb_data_type_t notmuch_rb_messages_type;
+extern const rb_data_type_t notmuch_rb_message_type;
+extern const rb_data_type_t notmuch_rb_tags_type;
+
+#define Data_Get_Notmuch_Object(obj, type, ptr)					    \
     do {									    \
-	(ptr) = rb_check_typeddata ((obj), &notmuch_rb_object_type);		    \
+	(ptr) = rb_check_typeddata ((obj), (type));				    \
 	if (RB_UNLIKELY (!(ptr))) {						    \
 	    VALUE cname = rb_class_name (CLASS_OF ((obj)));			    \
 	    rb_raise (rb_eRuntimeError, "%"PRIsVALUE" object destroyed", cname);    \
 	}									    \
     } while (0)
 
-#define Data_Wrap_Notmuch_Object(klass, ptr)	\
-    TypedData_Wrap_Struct ((klass), &notmuch_rb_object_type, (ptr))
+#define Data_Wrap_Notmuch_Object(klass, type, ptr) \
+    TypedData_Wrap_Struct ((klass), (type), (ptr))
 
 #define Data_Get_Notmuch_Database(obj, ptr) \
-    Data_Get_Notmuch_Object ((obj), (ptr))
+    Data_Get_Notmuch_Object ((obj), &notmuch_rb_database_type, (ptr))
 
 #define Data_Get_Notmuch_Directory(obj, ptr) \
-    Data_Get_Notmuch_Object ((obj), (ptr))
+    Data_Get_Notmuch_Object ((obj), &notmuch_rb_directory_type, (ptr))
 
 #define Data_Get_Notmuch_FileNames(obj, ptr) \
-    Data_Get_Notmuch_Object ((obj), (ptr))
+    Data_Get_Notmuch_Object ((obj), &notmuch_rb_filenames_type, (ptr))
 
 #define Data_Get_Notmuch_Query(obj, ptr) \
-    Data_Get_Notmuch_Object ((obj), (ptr))
+    Data_Get_Notmuch_Object ((obj), &notmuch_rb_query_type, (ptr))
 
 #define Data_Get_Notmuch_Threads(obj, ptr) \
-    Data_Get_Notmuch_Object ((obj), (ptr))
+    Data_Get_Notmuch_Object ((obj), &notmuch_rb_threads_type, (ptr))
 
 #define Data_Get_Notmuch_Messages(obj, ptr) \
-    Data_Get_Notmuch_Object ((obj), (ptr))
+    Data_Get_Notmuch_Object ((obj), &notmuch_rb_messages_type, (ptr))
 
 #define Data_Get_Notmuch_Thread(obj, ptr) \
-    Data_Get_Notmuch_Object ((obj), (ptr))
+    Data_Get_Notmuch_Object ((obj), &notmuch_rb_thread_type, (ptr))
 
 #define Data_Get_Notmuch_Message(obj, ptr) \
-    Data_Get_Notmuch_Object ((obj), (ptr))
+    Data_Get_Notmuch_Object ((obj), &notmuch_rb_message_type, (ptr))
 
 #define Data_Get_Notmuch_Tags(obj, ptr) \
-    Data_Get_Notmuch_Object ((obj), (ptr))
+    Data_Get_Notmuch_Object ((obj), &notmuch_rb_tags_type, (ptr))
 
 /* status.c */
 void
diff --git a/bindings/ruby/directory.c b/bindings/ruby/directory.c
index 36ef3984..17d60d1d 100644
--- a/bindings/ruby/directory.c
+++ b/bindings/ruby/directory.c
@@ -92,7 +92,7 @@ notmuch_rb_directory_get_child_files (VALUE self)
 
     fnames = notmuch_directory_get_child_files (dir);
 
-    return Data_Wrap_Notmuch_Object (notmuch_rb_cFileNames, fnames);
+    return Data_Wrap_Notmuch_Object (notmuch_rb_cFileNames, &notmuch_rb_filenames_type, fnames);
 }
 
 /*
@@ -111,5 +111,5 @@ notmuch_rb_directory_get_child_directories (VALUE self)
 
     fnames = notmuch_directory_get_child_directories (dir);
 
-    return Data_Wrap_Notmuch_Object (notmuch_rb_cFileNames, fnames);
+    return Data_Wrap_Notmuch_Object (notmuch_rb_cFileNames, &notmuch_rb_filenames_type, fnames);
 }
diff --git a/bindings/ruby/init.c b/bindings/ruby/init.c
index f3b2e5b1..a9f863eb 100644
--- a/bindings/ruby/init.c
+++ b/bindings/ruby/init.c
@@ -50,6 +50,22 @@ const rb_data_type_t notmuch_rb_object_type = {
     .wrap_struct_name = "notmuch_object",
 };
 
+#define define_type(id) \
+    const rb_data_type_t notmuch_rb_ ## id ## _type = { \
+	.wrap_struct_name = "notmuch_" #id, \
+	.parent = &notmuch_rb_object_type, \
+    }
+
+define_type (database);
+define_type (directory);
+define_type (filenames);
+define_type (query);
+define_type (threads);
+define_type (thread);
+define_type (messages);
+define_type (message);
+define_type (tags);
+
 /*
  * Document-module: Notmuch
  *
diff --git a/bindings/ruby/message.c b/bindings/ruby/message.c
index 1990bb97..b3aed604 100644
--- a/bindings/ruby/message.c
+++ b/bindings/ruby/message.c
@@ -89,7 +89,7 @@ notmuch_rb_message_get_replies (VALUE self)
 
     messages = notmuch_message_get_replies (message);
 
-    return Data_Wrap_Notmuch_Object (notmuch_rb_cMessages, messages);
+    return Data_Wrap_Notmuch_Object (notmuch_rb_cMessages, &notmuch_rb_messages_type, messages);
 }
 
 /*
@@ -125,7 +125,7 @@ notmuch_rb_message_get_filenames (VALUE self)
 
     fnames = notmuch_message_get_filenames (message);
 
-    return Data_Wrap_Notmuch_Object (notmuch_rb_cFileNames, fnames);
+    return Data_Wrap_Notmuch_Object (notmuch_rb_cFileNames, &notmuch_rb_filenames_type, fnames);
 }
 
 /*
@@ -226,7 +226,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, tags);
+    return Data_Wrap_Notmuch_Object (notmuch_rb_cTags, &notmuch_rb_tags_type, tags);
 }
 
 /*
diff --git a/bindings/ruby/messages.c b/bindings/ruby/messages.c
index 3d1669ee..e04f3af1 100644
--- a/bindings/ruby/messages.c
+++ b/bindings/ruby/messages.c
@@ -53,7 +53,7 @@ notmuch_rb_messages_each (VALUE self)
 
     for (; notmuch_messages_valid (messages); notmuch_messages_move_to_next (messages)) {
 	message = notmuch_messages_get (messages);
-	rb_yield (Data_Wrap_Notmuch_Object (notmuch_rb_cMessage, message));
+	rb_yield (Data_Wrap_Notmuch_Object (notmuch_rb_cMessage, &notmuch_rb_message_type, message));
     }
 
     return self;
@@ -76,5 +76,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, tags);
+    return Data_Wrap_Notmuch_Object (notmuch_rb_cTags, &notmuch_rb_tags_type, tags);
 }
diff --git a/bindings/ruby/query.c b/bindings/ruby/query.c
index b0fb4ea7..79727d6a 100644
--- a/bindings/ruby/query.c
+++ b/bindings/ruby/query.c
@@ -142,7 +142,7 @@ notmuch_rb_query_search_threads (VALUE self)
     if (status)
 	notmuch_rb_status_raise (status);
 
-    return Data_Wrap_Notmuch_Object (notmuch_rb_cThreads, threads);
+    return Data_Wrap_Notmuch_Object (notmuch_rb_cThreads, &notmuch_rb_threads_type, threads);
 }
 
 /*
@@ -163,7 +163,7 @@ notmuch_rb_query_search_messages (VALUE self)
     if (status)
 	notmuch_rb_status_raise (status);
 
-    return Data_Wrap_Notmuch_Object (notmuch_rb_cMessages, messages);
+    return Data_Wrap_Notmuch_Object (notmuch_rb_cMessages, &notmuch_rb_messages_type, messages);
 }
 
 /*
diff --git a/bindings/ruby/thread.c b/bindings/ruby/thread.c
index e09be147..f6bf7849 100644
--- a/bindings/ruby/thread.c
+++ b/bindings/ruby/thread.c
@@ -88,7 +88,7 @@ notmuch_rb_thread_get_toplevel_messages (VALUE self)
     if (!messages)
 	rb_raise (notmuch_rb_eMemoryError, "Out of memory");
 
-    return Data_Wrap_Notmuch_Object (notmuch_rb_cMessages, messages);
+    return Data_Wrap_Notmuch_Object (notmuch_rb_cMessages, &notmuch_rb_messages_type, messages);
 }
 
 /*
@@ -108,7 +108,7 @@ notmuch_rb_thread_get_messages (VALUE self)
     if (!messages)
 	rb_raise (notmuch_rb_eMemoryError, "Out of memory");
 
-    return Data_Wrap_Notmuch_Object (notmuch_rb_cMessages, messages);
+    return Data_Wrap_Notmuch_Object (notmuch_rb_cMessages, &notmuch_rb_messages_type, messages);
 }
 
 /*
@@ -209,5 +209,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, tags);
+    return Data_Wrap_Notmuch_Object (notmuch_rb_cTags, &notmuch_rb_tags_type, tags);
 }
diff --git a/bindings/ruby/threads.c b/bindings/ruby/threads.c
index 19e09a7e..d809b571 100644
--- a/bindings/ruby/threads.c
+++ b/bindings/ruby/threads.c
@@ -53,7 +53,7 @@ notmuch_rb_threads_each (VALUE self)
 
     for (; notmuch_threads_valid (threads); notmuch_threads_move_to_next (threads)) {
 	thread = notmuch_threads_get (threads);
-	rb_yield (Data_Wrap_Notmuch_Object (notmuch_rb_cThread, thread));
+	rb_yield (Data_Wrap_Notmuch_Object (notmuch_rb_cThread, &notmuch_rb_thread_type, thread));
     }
 
     return self;
-- 
2.31.0

[PATCH v2 10/10] ruby: new notmuch_rb_object_destroy() helper

[Felipe Contreras]

Using the rb_data_type_t data we can call the correct notmuch destroy
function.

Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
---
 bindings/ruby/database.c  |  6 +-----
 bindings/ruby/defs.h      | 14 ++++++++++++++
 bindings/ruby/directory.c |  7 +------
 bindings/ruby/filenames.c |  7 +------
 bindings/ruby/init.c      |  1 +
 bindings/ruby/message.c   |  7 +------
 bindings/ruby/messages.c  |  7 +------
 bindings/ruby/query.c     |  7 +------
 bindings/ruby/tags.c      |  7 +------
 bindings/ruby/thread.c    |  7 +------
 bindings/ruby/threads.c   |  7 +------
 11 files changed, 24 insertions(+), 53 deletions(-)

diff --git a/bindings/ruby/database.c b/bindings/ruby/database.c
index 4ecc8f78..bb993d86 100644
--- a/bindings/ruby/database.c
+++ b/bindings/ruby/database.c
@@ -114,11 +114,7 @@ VALUE
 notmuch_rb_database_close (VALUE self)
 {
     notmuch_status_t ret;
-    notmuch_database_t *db;
-
-    Data_Get_Notmuch_Database (self, db);
-    ret = notmuch_database_destroy (db);
-    DATA_PTR (self) = NULL;
+    ret = notmuch_rb_object_destroy (self, &notmuch_rb_database_type);
     notmuch_rb_status_raise (ret);
 
     return Qnil;
diff --git a/bindings/ruby/defs.h b/bindings/ruby/defs.h
index fa7b9515..66b476e2 100644
--- a/bindings/ruby/defs.h
+++ b/bindings/ruby/defs.h
@@ -105,6 +105,20 @@ extern const rb_data_type_t notmuch_rb_tags_type;
 #define Data_Get_Notmuch_Tags(obj, ptr) \
     Data_Get_Notmuch_Object ((obj), &notmuch_rb_tags_type, (ptr))
 
+static inline notmuch_status_t
+notmuch_rb_object_destroy (VALUE rb_object, const rb_data_type_t *type)
+{
+    void *nm_object;
+    notmuch_status_t ret;
+
+    Data_Get_Notmuch_Object (rb_object, type, nm_object);
+
+    ret = ((notmuch_status_t (*)(void *)) type->data) (nm_object);
+    DATA_PTR (rb_object) = NULL;
+
+    return ret;
+}
+
 /* status.c */
 void
 notmuch_rb_status_raise (notmuch_status_t status);
diff --git a/bindings/ruby/directory.c b/bindings/ruby/directory.c
index 17d60d1d..910f0a99 100644
--- a/bindings/ruby/directory.c
+++ b/bindings/ruby/directory.c
@@ -28,12 +28,7 @@
 VALUE
 notmuch_rb_directory_destroy (VALUE self)
 {
-    notmuch_directory_t *dir;
-
-    Data_Get_Notmuch_Directory (self, dir);
-
-    notmuch_directory_destroy (dir);
-    DATA_PTR (self) = NULL;
+    notmuch_rb_object_destroy (self, &notmuch_rb_directory_type);
 
     return Qnil;
 }
diff --git a/bindings/ruby/filenames.c b/bindings/ruby/filenames.c
index 656c58e6..0dec1952 100644
--- a/bindings/ruby/filenames.c
+++ b/bindings/ruby/filenames.c
@@ -28,12 +28,7 @@
 VALUE
 notmuch_rb_filenames_destroy (VALUE self)
 {
-    notmuch_filenames_t *fnames;
-
-    Data_Get_Notmuch_FileNames (self, fnames);
-
-    notmuch_filenames_destroy (fnames);
-    DATA_PTR (self) = NULL;
+    notmuch_rb_object_destroy (self, &notmuch_rb_filenames_type);
 
     return Qnil;
 }
diff --git a/bindings/ruby/init.c b/bindings/ruby/init.c
index a9f863eb..62515eca 100644
--- a/bindings/ruby/init.c
+++ b/bindings/ruby/init.c
@@ -54,6 +54,7 @@ const rb_data_type_t notmuch_rb_object_type = {
     const rb_data_type_t notmuch_rb_ ## id ## _type = { \
 	.wrap_struct_name = "notmuch_" #id, \
 	.parent = &notmuch_rb_object_type, \
+	.data = &notmuch_ ## id ## _destroy, \
     }
 
 define_type (database);
diff --git a/bindings/ruby/message.c b/bindings/ruby/message.c
index b3aed604..f45c95cc 100644
--- a/bindings/ruby/message.c
+++ b/bindings/ruby/message.c
@@ -28,12 +28,7 @@
 VALUE
 notmuch_rb_message_destroy (VALUE self)
 {
-    notmuch_message_t *message;
-
-    Data_Get_Notmuch_Message (self, message);
-
-    notmuch_message_destroy (message);
-    DATA_PTR (self) = NULL;
+    notmuch_rb_object_destroy (self, &notmuch_rb_message_type);
 
     return Qnil;
 }
diff --git a/bindings/ruby/messages.c b/bindings/ruby/messages.c
index e04f3af1..ca5b10d0 100644
--- a/bindings/ruby/messages.c
+++ b/bindings/ruby/messages.c
@@ -28,12 +28,7 @@
 VALUE
 notmuch_rb_messages_destroy (VALUE self)
 {
-    notmuch_messages_t *messages;
-
-    Data_Get_Notmuch_Messages (self, messages);
-
-    notmuch_messages_destroy (messages);
-    DATA_PTR (self) = NULL;
+    notmuch_rb_object_destroy (self, &notmuch_rb_messages_type);
 
     return Qnil;
 }
diff --git a/bindings/ruby/query.c b/bindings/ruby/query.c
index 79727d6a..3ec98c6c 100644
--- a/bindings/ruby/query.c
+++ b/bindings/ruby/query.c
@@ -28,12 +28,7 @@
 VALUE
 notmuch_rb_query_destroy (VALUE self)
 {
-    notmuch_query_t *query;
-
-    Data_Get_Notmuch_Query (self, query);
-
-    notmuch_query_destroy (query);
-    DATA_PTR (self) = NULL;
+    notmuch_rb_object_destroy (self, &notmuch_rb_query_type);
 
     return Qnil;
 }
diff --git a/bindings/ruby/tags.c b/bindings/ruby/tags.c
index db8b4cfc..2af85e36 100644
--- a/bindings/ruby/tags.c
+++ b/bindings/ruby/tags.c
@@ -28,12 +28,7 @@
 VALUE
 notmuch_rb_tags_destroy (VALUE self)
 {
-    notmuch_tags_t *tags;
-
-    Data_Get_Notmuch_Tags (self, tags);
-
-    notmuch_tags_destroy (tags);
-    DATA_PTR (self) = NULL;
+    notmuch_rb_object_destroy (self, &notmuch_rb_tags_type);
 
     return Qnil;
 }
diff --git a/bindings/ruby/thread.c b/bindings/ruby/thread.c
index f6bf7849..7cb2a3dc 100644
--- a/bindings/ruby/thread.c
+++ b/bindings/ruby/thread.c
@@ -28,12 +28,7 @@
 VALUE
 notmuch_rb_thread_destroy (VALUE self)
 {
-    notmuch_thread_t *thread;
-
-    Data_Get_Notmuch_Thread (self, thread);
-
-    notmuch_thread_destroy (thread);
-    DATA_PTR (self) = NULL;
+    notmuch_rb_object_destroy (self, &notmuch_rb_thread_type);
 
     return Qnil;
 }
diff --git a/bindings/ruby/threads.c b/bindings/ruby/threads.c
index d809b571..50280260 100644
--- a/bindings/ruby/threads.c
+++ b/bindings/ruby/threads.c
@@ -28,12 +28,7 @@
 VALUE
 notmuch_rb_threads_destroy (VALUE self)
 {
-    notmuch_threads_t *threads;
-
-    Data_Get_Notmuch_Threads (self, threads);
-
-    notmuch_threads_destroy (threads);
-    DATA_PTR (self) = NULL;
+    notmuch_rb_object_destroy (self, &notmuch_rb_threads_type);
 
     return Qnil;
 }
-- 
2.31.0

Re: [PATCH v2 10/10] ruby: new notmuch_rb_object_destroy() helper

[David Bremner]

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

> +static inline notmuch_status_t
> +notmuch_rb_object_destroy (VALUE rb_object, const rb_data_type_t *type)
> +{
> +    void *nm_object;
> +    notmuch_status_t ret;
> +
> +    Data_Get_Notmuch_Object (rb_object, type, nm_object);
> +
> +    ret = ((notmuch_status_t (*)(void *)) type->data) (nm_object);
> +    DATA_PTR (rb_object) = NULL;
> +
> +    return ret;
> +}
> +

I see the benefit of making the code shorter, but I don't understand the
new code, while I did (mostly) understand the old code. So please
explain this in a comment as if I have no idea how ruby extensions work.

d

Re: [PATCH v2 10/10] ruby: new notmuch_rb_object_destroy() helper

[Felipe Contreras]

On Wed, May 12, 2021 at 5:10 PM David Bremner <david@tethera.net> wrote:
>
> Felipe Contreras <felipe.contreras@gmail.com> writes:
>
> > +static inline notmuch_status_t
> > +notmuch_rb_object_destroy (VALUE rb_object, const rb_data_type_t *type)
> > +{
> > +    void *nm_object;
> > +    notmuch_status_t ret;
> > +
> > +    Data_Get_Notmuch_Object (rb_object, type, nm_object);
> > +
> > +    ret = ((notmuch_status_t (*)(void *)) type->data) (nm_object);
> > +    DATA_PTR (rb_object) = NULL;
> > +
> > +    return ret;
> > +}
> > +
>
> I see the benefit of making the code shorter, but I don't understand the
> new code, while I did (mostly) understand the old code. So please
> explain this in a comment as if I have no idea how ruby extensions work.

The struct used to store the types (rb_data_type_t) contains a "data"
field where we can store whatever we want. I use that field to store a
pointer to the corresponding destroy function. For example
notmuch_rb_database_type contains a pointer to
notmuch_database_destroy.

I cast that pointer as a notmuch_status_t (func*)(void *) and call
that function passing the internal object (e.g. notmuch_database_t).

Is that explanation good enough?

Cheers.

-- 
Felipe Contreras

Re: [PATCH v2 00/10] ruby: object cleanups

[Felipe Contreras]

On Tue, May 4, 2021 at 3:17 AM Felipe Contreras
<felipe.contreras@gmail.com> wrote:
>
> This superseeds my previous series [1] with much more cleanups.
>
> An important new change is the move towards RTypedData, which is way superior to the old RData
> objects.
>
> Everything should work basically the same though.

Please remove Austin Clements from the CC list, his address doesn't
work anymore.

-- 
Felipe Contreras

Re: [PATCH v2 00/10] ruby: object cleanups

[Felipe Contreras]

On Tue, May 4, 2021 at 3:25 AM Felipe Contreras
<felipe.contreras@gmail.com> wrote:
>
> On Tue, May 4, 2021 at 3:17 AM Felipe Contreras
> <felipe.contreras@gmail.com> wrote:
> >
> > This superseeds my previous series [1] with much more cleanups.
> >
> > An important new change is the move towards RTypedData, which is way superior to the old RData
> > objects.
> >
> > Everything should work basically the same though.
>
> Please remove Austin Clements from the CC list, his address doesn't
> work anymore.

What's the status with these? Shall I split this series of 10 patches
into multiple smaller patch series in order to reduce the reviewing
burden?

-- 
Felipe Contreras

Re: [PATCH v2 00/10] ruby: object cleanups

[Tomi Ollila]

On Tue, May 11 2021, Felipe Contreras wrote:

> On Tue, May 4, 2021 at 3:25 AM Felipe Contreras
> <felipe.contreras@gmail.com> wrote:
>>
>> On Tue, May 4, 2021 at 3:17 AM Felipe Contreras
>> <felipe.contreras@gmail.com> wrote:
>> >
>> > This superseeds my previous series [1] with much more cleanups.
>> >
>> > An important new change is the move towards RTypedData, which is way superior to the old RData
>> > objects.
>> >
>> > Everything should work basically the same though.
>>
>> Please remove Austin Clements from the CC list, his address doesn't
>> work anymore.
>
> What's the status with these? Shall I split this series of 10 patches
> into multiple smaller patch series in order to reduce the reviewing
> burden?

I would not say it is burden to review the current series; just 
to find someone capable with time to test it (I personally only
use notmuch cli and emacs client, no skills w/ anything else
(than "trivial" things))

Tomi

>
> -- 
> Felipe Contreras