unofficial mirror of notmuch@notmuchmail.org
 help / color / mirror / code / Atom feed
* [PATCH v3 0/7] ruby: object cleanups
@ 2021-05-15 21:20 Felipe Contreras
  2021-05-15 21:20 ` [PATCH v3 1/7] ruby: simplify data get helper Felipe Contreras
                   ` (6 more replies)
  0 siblings, 7 replies; 9+ messages in thread
From: Felipe Contreras @ 2021-05-15 21:20 UTC (permalink / raw)
  To: notmuch
  Cc: Ali Polatel, Austin Clements, Tomi Ollila, Ludovic LANGE,
	Daniel Kahn Gillmor

This is basically the same as v2, except I updated the commit messages as David Bremner suggested,
and I added a minor comment.

Felipe Contreras (7):
  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      | 59 +++++++++++++++++++++++++++++----------
 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, 96 insertions(+), 88 deletions(-)

Range-diff against v2:
 1:  9c15fc44 <  -:  -------- ruby: add missing Data_Get_Notmuch helpers
 2:  c9d840d3 <  -:  -------- ruby: improve all Data_Get_Notmuch_* helpers
 3:  299b2be1 <  -:  -------- ruby: improve general data get helper
 4:  19fa26de !  1:  a1dc3960 ruby: simplify data get helper
    @@ Metadata
      ## Commit message ##
         ruby: simplify data get helper
     
    -    The type is not actually needed.
    +    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)))
    +
    +    We can use rb_data_object_get directly, and this way we don't need to
    +    pass the type, which is unnecessary information.
     
         Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
     
 5:  1872c4b5 =  2:  2f88cf0a ruby: fetch class name in case of error
 6:  b46bcac8 =  3:  6671b155 ruby: add unlikely hint
 7:  1bfa0334 =  4:  87222fd0 ruby: create Data_Wrap_Notmuch_Object helper
 8:  a9b7ac45 !  5:  1a3866c1 ruby: move towards more modern RTypedData
    @@ Metadata
      ## Commit message ##
         ruby: move towards more modern RTypedData
     
    +    Virtually the whole ruby core moved from RData to RTypeData, let's do so
    +    ourselves too.
    +
    +    Basically the information typically passed through Data_Wrap_Struct is
    +    now stored in a struct rb_data_type_t (mark and free functions). This
    +    has the advantage that more information can be easily added, like the
    +    name of the type, a custom data ponter, and more.
    +
    +    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).
    +
    +            #define Check_TypedStruct(v, t)      \
    +                rb_check_typeddata(RBIMPL_CAST((VALUE)(v)), (t))
    +
    +    We can use rb_check_typeddata directly, just like we use rb_data_object_get
    +    directly.
    +
         Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
     
      ## bindings/ruby/database.c ##
 9:  e2116d5d =  6:  b3366e12 ruby: add all data types
10:  f6660e5b !  7:  05795b05 ruby: new notmuch_rb_object_destroy() helper
    @@ Metadata
      ## Commit message ##
         ruby: new notmuch_rb_object_destroy() helper
     
    +    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).
    +
         Using the rb_data_type_t data we can call the correct notmuch destroy
         function.
     
    +    Therefore this:
    +
    +      ret = ((notmuch_status_t (*)(void *)) type->data) (nm_object);
    +
    +    Is effectively the same as this:
    +
    +      ret = notmuch_database_destroy (database);
    +
    +    The advantage of doing it this way is that much less code is necesary
    +    since each rb_data_type_t has the corresponding destroy function stored
    +    in it.
    +
         Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
     
      ## bindings/ruby/database.c ##
    @@ bindings/ruby/defs.h: extern const rb_data_type_t notmuch_rb_tags_type;
     +
     +    Data_Get_Notmuch_Object (rb_object, type, nm_object);
     +
    ++    /* Call the corresponding notmuch_*_destroy function */
     +    ret = ((notmuch_status_t (*)(void *)) type->data) (nm_object);
     +    DATA_PTR (rb_object) = NULL;
     +
-- 
2.31.1

^ permalink raw reply	[flat|nested] 9+ messages in thread

* [PATCH v3 1/7] ruby: simplify data get helper
  2021-05-15 21:20 [PATCH v3 0/7] ruby: object cleanups Felipe Contreras
@ 2021-05-15 21:20 ` Felipe Contreras
  2021-05-17 10:55   ` David Bremner
  2021-05-15 21:20 ` [PATCH v3 2/7] ruby: fetch class name in case of error Felipe Contreras
                   ` (5 subsequent siblings)
  6 siblings, 1 reply; 9+ messages in thread
From: Felipe Contreras @ 2021-05-15 21:20 UTC (permalink / raw)
  To: notmuch
  Cc: Ali Polatel, Austin Clements, Tomi Ollila, Ludovic LANGE,
	Daniel Kahn Gillmor

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

We can use rb_data_object_get directly, and this way we don't need to
pass the type, which is unnecessary information.

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.1

^ permalink raw reply	[flat|nested] 9+ messages in thread

* [PATCH v3 2/7] ruby: fetch class name in case of error
  2021-05-15 21:20 [PATCH v3 0/7] ruby: object cleanups Felipe Contreras
  2021-05-15 21:20 ` [PATCH v3 1/7] ruby: simplify data get helper Felipe Contreras
@ 2021-05-15 21:20 ` Felipe Contreras
  2021-05-15 21:20 ` [PATCH v3 3/7] ruby: add unlikely hint Felipe Contreras
                   ` (4 subsequent siblings)
  6 siblings, 0 replies; 9+ messages in thread
From: Felipe Contreras @ 2021-05-15 21:20 UTC (permalink / raw)
  To: notmuch
  Cc: Ali Polatel, Austin Clements, Tomi Ollila, Ludovic LANGE,
	Daniel Kahn Gillmor

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.1

^ permalink raw reply	[flat|nested] 9+ messages in thread

* [PATCH v3 3/7] ruby: add unlikely hint
  2021-05-15 21:20 [PATCH v3 0/7] ruby: object cleanups Felipe Contreras
  2021-05-15 21:20 ` [PATCH v3 1/7] ruby: simplify data get helper Felipe Contreras
  2021-05-15 21:20 ` [PATCH v3 2/7] ruby: fetch class name in case of error Felipe Contreras
@ 2021-05-15 21:20 ` Felipe Contreras
  2021-05-15 21:21 ` [PATCH v3 4/7] ruby: create Data_Wrap_Notmuch_Object helper Felipe Contreras
                   ` (3 subsequent siblings)
  6 siblings, 0 replies; 9+ messages in thread
From: Felipe Contreras @ 2021-05-15 21:20 UTC (permalink / raw)
  To: notmuch
  Cc: Ali Polatel, Austin Clements, Tomi Ollila, Ludovic LANGE,
	Daniel Kahn Gillmor

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.1

^ permalink raw reply	[flat|nested] 9+ messages in thread

* [PATCH v3 4/7] ruby: create Data_Wrap_Notmuch_Object helper
  2021-05-15 21:20 [PATCH v3 0/7] ruby: object cleanups Felipe Contreras
                   ` (2 preceding siblings ...)
  2021-05-15 21:20 ` [PATCH v3 3/7] ruby: add unlikely hint Felipe Contreras
@ 2021-05-15 21:21 ` Felipe Contreras
  2021-05-15 21:21 ` [PATCH v3 5/7] ruby: move towards more modern RTypedData Felipe Contreras
                   ` (2 subsequent siblings)
  6 siblings, 0 replies; 9+ messages in thread
From: Felipe Contreras @ 2021-05-15 21:21 UTC (permalink / raw)
  To: notmuch
  Cc: Ali Polatel, Austin Clements, Tomi Ollila, Ludovic LANGE,
	Daniel Kahn Gillmor

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.1

^ permalink raw reply	[flat|nested] 9+ messages in thread

* [PATCH v3 5/7] ruby: move towards more modern RTypedData
  2021-05-15 21:20 [PATCH v3 0/7] ruby: object cleanups Felipe Contreras
                   ` (3 preceding siblings ...)
  2021-05-15 21:21 ` [PATCH v3 4/7] ruby: create Data_Wrap_Notmuch_Object helper Felipe Contreras
@ 2021-05-15 21:21 ` Felipe Contreras
  2021-05-15 21:21 ` [PATCH v3 6/7] ruby: add all data types Felipe Contreras
  2021-05-15 21:21 ` [PATCH v3 7/7] ruby: new notmuch_rb_object_destroy() helper Felipe Contreras
  6 siblings, 0 replies; 9+ messages in thread
From: Felipe Contreras @ 2021-05-15 21:21 UTC (permalink / raw)
  To: notmuch
  Cc: Ali Polatel, Austin Clements, Tomi Ollila, Ludovic LANGE,
	Daniel Kahn Gillmor

Virtually the whole ruby core moved from RData to RTypeData, let's do so
ourselves too.

Basically the information typically passed through Data_Wrap_Struct is
now stored in a struct rb_data_type_t (mark and free functions). This
has the advantage that more information can be easily added, like the
name of the type, a custom data ponter, and more.

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).

        #define Check_TypedStruct(v, t)      \
            rb_check_typeddata(RBIMPL_CAST((VALUE)(v)), (t))

We can use rb_check_typeddata directly, just like we use rb_data_object_get
directly.

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.1

^ permalink raw reply	[flat|nested] 9+ messages in thread

* [PATCH v3 6/7] ruby: add all data types
  2021-05-15 21:20 [PATCH v3 0/7] ruby: object cleanups Felipe Contreras
                   ` (4 preceding siblings ...)
  2021-05-15 21:21 ` [PATCH v3 5/7] ruby: move towards more modern RTypedData Felipe Contreras
@ 2021-05-15 21:21 ` Felipe Contreras
  2021-05-15 21:21 ` [PATCH v3 7/7] ruby: new notmuch_rb_object_destroy() helper Felipe Contreras
  6 siblings, 0 replies; 9+ messages in thread
From: Felipe Contreras @ 2021-05-15 21:21 UTC (permalink / raw)
  To: notmuch
  Cc: Ali Polatel, Austin Clements, Tomi Ollila, Ludovic LANGE,
	Daniel Kahn Gillmor

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.1

^ permalink raw reply	[flat|nested] 9+ messages in thread

* [PATCH v3 7/7] ruby: new notmuch_rb_object_destroy() helper
  2021-05-15 21:20 [PATCH v3 0/7] ruby: object cleanups Felipe Contreras
                   ` (5 preceding siblings ...)
  2021-05-15 21:21 ` [PATCH v3 6/7] ruby: add all data types Felipe Contreras
@ 2021-05-15 21:21 ` Felipe Contreras
  6 siblings, 0 replies; 9+ messages in thread
From: Felipe Contreras @ 2021-05-15 21:21 UTC (permalink / raw)
  To: notmuch
  Cc: Ali Polatel, Austin Clements, Tomi Ollila, Ludovic LANGE,
	Daniel Kahn Gillmor

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).

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

Therefore this:

  ret = ((notmuch_status_t (*)(void *)) type->data) (nm_object);

Is effectively the same as this:

  ret = notmuch_database_destroy (database);

The advantage of doing it this way is that much less code is necesary
since each rb_data_type_t has the corresponding destroy function stored
in it.

Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
---
 bindings/ruby/database.c  |  6 +-----
 bindings/ruby/defs.h      | 15 +++++++++++++++
 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, 25 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..9860ee17 100644
--- a/bindings/ruby/defs.h
+++ b/bindings/ruby/defs.h
@@ -105,6 +105,21 @@ 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);
+
+    /* Call the corresponding notmuch_*_destroy function */
+    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.1

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH v3 1/7] ruby: simplify data get helper
  2021-05-15 21:20 ` [PATCH v3 1/7] ruby: simplify data get helper Felipe Contreras
@ 2021-05-17 10:55   ` David Bremner
  0 siblings, 0 replies; 9+ messages in thread
From: David Bremner @ 2021-05-17 10:55 UTC (permalink / raw)
  To: Felipe Contreras, notmuch

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

> 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)))
>
> We can use rb_data_object_get directly, and this way we don't need to
> pass the type, which is unnecessary information.

Thanks for the updated commit messages. Applied to master

d

^ permalink raw reply	[flat|nested] 9+ messages in thread

end of thread, other threads:[~2021-05-17 10:55 UTC | newest]

Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-05-15 21:20 [PATCH v3 0/7] ruby: object cleanups Felipe Contreras
2021-05-15 21:20 ` [PATCH v3 1/7] ruby: simplify data get helper Felipe Contreras
2021-05-17 10:55   ` David Bremner
2021-05-15 21:20 ` [PATCH v3 2/7] ruby: fetch class name in case of error Felipe Contreras
2021-05-15 21:20 ` [PATCH v3 3/7] ruby: add unlikely hint Felipe Contreras
2021-05-15 21:21 ` [PATCH v3 4/7] ruby: create Data_Wrap_Notmuch_Object helper Felipe Contreras
2021-05-15 21:21 ` [PATCH v3 5/7] ruby: move towards more modern RTypedData Felipe Contreras
2021-05-15 21:21 ` [PATCH v3 6/7] ruby: add all data types Felipe Contreras
2021-05-15 21:21 ` [PATCH v3 7/7] ruby: new notmuch_rb_object_destroy() helper Felipe Contreras

unofficial mirror of notmuch@notmuchmail.org

This inbox may be cloned and mirrored by anyone:

	git clone --mirror https://yhetil.org/notmuch/0 notmuch/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 notmuch notmuch/ https://yhetil.org/notmuch \
		notmuch@notmuchmail.org
	public-inbox-index notmuch

Example config snippet for mirrors.
Newsgroups are available over NNTP:
	nntp://news.yhetil.org/yhetil.mail.notmuch.general
	nntp://news.gmane.io/gmane.mail.notmuch.general


code repositories for project(s) associated with this inbox:

	notmuch.git.git (no URL configured)

AGPL code for this site: git clone http://ou63pmih66umazou.onion/public-inbox.git