[PATCH 0/3] General fixes

[PATCH 0/3] General fixes

[Felipe Contreras]

Felipe Contreras (3):
  query: bind queries to database objects
  ruby: allow build with RUNPATH
  ruby: bind database close()/destroy() properly

 bindings/ruby/database.c | 22 +++++++++++++++++++---
 bindings/ruby/defs.h     |  5 ++++-
 bindings/ruby/extconf.rb |  3 +++
 bindings/ruby/init.c     |  1 +
 lib/query.cc             |  2 +-
 5 files changed, 28 insertions(+), 5 deletions(-)

-- 
1.8.4-fc

[PATCH 1/3] query: bind queries to database objects

[Felipe Contreras]

The queries don't really work after a database is closed, and we would
like them to be freed if the database is destroyed.

Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
---
 lib/query.cc | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/lib/query.cc b/lib/query.cc
index 69668a4..ec60e2e 100644
--- a/lib/query.cc
+++ b/lib/query.cc
@@ -80,7 +80,7 @@ notmuch_query_create (notmuch_database_t *notmuch,
     if (_debug_query ())
 	fprintf (stderr, "Query string is:\n%s\n", query_string);
 
-    query = talloc (NULL, notmuch_query_t);
+    query = talloc (notmuch, notmuch_query_t);
     if (unlikely (query == NULL))
 	return NULL;
 
-- 
1.8.4-fc

[PATCH 2/3] ruby: allow build with RUNPATH

[Felipe Contreras]

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

diff --git a/bindings/ruby/extconf.rb b/bindings/ruby/extconf.rb
index 6160db2..778016f 100644
--- a/bindings/ruby/extconf.rb
+++ b/bindings/ruby/extconf.rb
@@ -13,6 +13,9 @@ $INCFLAGS = "-I#{dir} #{$INCFLAGS}"
 # make sure there are no undefined symbols
 $LDFLAGS += ' -Wl,--no-undefined'
 
+# enable RUNPATH
+$LDFLAGS += ' -Wl,--enable-new-dtags'
+
 def have_local_library(lib, path, func, headers = nil)
   checking_for checking_message(func, lib) do
     lib = File.join(path, lib)
-- 
1.8.4-fc

[PATCH 3/3] ruby: bind database close()/destroy() properly

[Felipe Contreras]

db.close() should close, and db.destroy!() should destroy, it's a simple
as that.

In addition, this makes is reasonable to allow the database objects to
be garbage collected, and if the database object is destroyed, all the
dependent objects are destroyed as well, so foo.destroy!() are not
necessary.

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

diff --git a/bindings/ruby/database.c b/bindings/ruby/database.c
index e84f726..8f2e16d 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_Struct (klass, NULL, notmuch_database_destroy, NULL);
 }
 
 /*
@@ -87,6 +87,23 @@ notmuch_rb_database_initialize (int argc, VALUE *argv, VALUE self)
 }
 
 /*
+ * call-seq: DB.destroy! => nil
+ *
+ * Destroy the notmuch database, freeing all resources allocated for it.
+ */
+VALUE
+notmuch_rb_database_destroy (VALUE self)
+{
+    notmuch_database_t *db;
+
+    Data_Get_Notmuch_Database (self, db);
+    notmuch_database_destroy (db);
+    DATA_PTR (self) = NULL;
+
+    return Qnil;
+}
+
+/*
  * call-seq: Notmuch::Database.open(path [, ahash]) {|db| ...}
  *
  * Identical to new, except that when it is called with a block, it yields with
@@ -116,8 +133,7 @@ notmuch_rb_database_close (VALUE self)
     notmuch_database_t *db;
 
     Data_Get_Notmuch_Database (self, db);
-    notmuch_database_destroy (db);
-    DATA_PTR (self) = NULL;
+    notmuch_database_close (db);
 
     return Qnil;
 }
diff --git a/bindings/ruby/defs.h b/bindings/ruby/defs.h
index 5b44585..0ad198a 100644
--- a/bindings/ruby/defs.h
+++ b/bindings/ruby/defs.h
@@ -59,7 +59,7 @@ extern ID ID_db_mode;
     do {							\
 	Check_Type ((obj), T_DATA);				\
 	if (DATA_PTR ((obj)) == NULL)				\
-	rb_raise (rb_eRuntimeError, "database closed");		\
+	rb_raise (rb_eRuntimeError, "database destroyed");	\
 	Data_Get_Struct ((obj), notmuch_database_t, (ptr));	\
     } while (0)
 
@@ -139,6 +139,9 @@ VALUE
 notmuch_rb_database_initialize (int argc, VALUE *argv, VALUE klass);
 
 VALUE
+notmuch_rb_database_destroy (VALUE self);
+
+VALUE
 notmuch_rb_database_open (int argc, VALUE *argv, VALUE klass);
 
 VALUE
diff --git a/bindings/ruby/init.c b/bindings/ruby/init.c
index 663271d..fee04c3 100644
--- a/bindings/ruby/init.c
+++ b/bindings/ruby/init.c
@@ -215,6 +215,7 @@ Init_notmuch (void)
     rb_define_alloc_func (notmuch_rb_cDatabase, notmuch_rb_database_alloc);
     rb_define_singleton_method (notmuch_rb_cDatabase, "open", notmuch_rb_database_open, -1); /* in database.c */
     rb_define_method (notmuch_rb_cDatabase, "initialize", notmuch_rb_database_initialize, -1); /* in database.c */
+    rb_define_method (notmuch_rb_cDatabase, "destroy!", notmuch_rb_database_destroy, 0); /* in database.c */
     rb_define_method (notmuch_rb_cDatabase, "close", notmuch_rb_database_close, 0); /* in database.c */
     rb_define_method (notmuch_rb_cDatabase, "path", notmuch_rb_database_path, 0); /* in database.c */
     rb_define_method (notmuch_rb_cDatabase, "version", notmuch_rb_database_version, 0); /* in database.c */
-- 
1.8.4-fc

Re: [PATCH 0/3] General fixes

[David Bremner]

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

> Felipe Contreras (3):
>   query: bind queries to database objects
>   ruby: allow build with RUNPATH
>   ruby: bind database close()/destroy() properly

Hi Felipe;

I agree with the discussion on IRC that the change in the first patch
makes sense.

It would be nice to have a bit more explanation in the commit message of
the second message; at least to point out that (iiuc) this is not a
really a ruby specific thing, just a standard ld.so feature.

d

Re: [PATCH 0/3] General fixes

[Felipe Contreras]

On Thu, Oct 10, 2013 at 6:02 AM, David Bremner <david@tethera.net> wrote:
> Felipe Contreras <felipe.contreras@gmail.com> writes:
>
>> Felipe Contreras (3):
>>   query: bind queries to database objects
>>   ruby: allow build with RUNPATH
>>   ruby: bind database close()/destroy() properly
>
> Hi Felipe;
>
> I agree with the discussion on IRC that the change in the first patch
> makes sense.
>
> It would be nice to have a bit more explanation in the commit message of
> the second message; at least to point out that (iiuc) this is not a
> really a ruby specific thing, just a standard ld.so feature.

Yes, that is true, and an option libnotmuch.so already uses, so I
thought it made sense that Ruby's notmuch.so used it as well.

-- 
Felipe Contreras

Re: [PATCH 0/3] General fixes

[Felipe Contreras]

David Bremner wrote:
> Felipe Contreras <felipe.contreras@gmail.com> writes:
> 
> > Felipe Contreras (3):
> >   query: bind queries to database objects
> >   ruby: allow build with RUNPATH
> >   ruby: bind database close()/destroy() properly
> 
> I agree with the discussion on IRC that the change in the first patch
> makes sense.

Shall I push it to the master branch then?

> It would be nice to have a bit more explanation in the commit message of
> the second message; at least to point out that (iiuc) this is not a
> really a ruby specific thing, just a standard ld.so feature.

I don't really care that much about patch #2, but #3 should probably be
applied.

The important is #1 though.

-- 
Felipe Contreras

Re: [PATCH 0/3] General fixes

[David Bremner]

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

> David Bremner wrote:
>> Felipe Contreras <felipe.contreras@gmail.com> writes:
>> 
>> > Felipe Contreras (3):
>> >   query: bind queries to database objects
>> >   ruby: allow build with RUNPATH
>> >   ruby: bind database close()/destroy() properly
>> 
>> I agree with the discussion on IRC that the change in the first patch
>> makes sense.
>
> Shall I push it to the master branch then?

sure.

> I don't really care that much about patch #2, but #3 should probably be
> applied.

Just to be clear, I wasn't objecting to patch 2, just asking for a few
more words of commit message. For patch 3, since it's ruby specific and
nobody complained, I'd say go for it.

Cheers,

d

Re: [PATCH 0/3] General fixes

[Felipe Contreras]

David Bremner wrote:
> Felipe Contreras <felipe.contreras@gmail.com> writes:
> 
> > David Bremner wrote:
> >> Felipe Contreras <felipe.contreras@gmail.com> writes:
> >> 
> >> > Felipe Contreras (3):
> >> >   query: bind queries to database objects
> >> >   ruby: allow build with RUNPATH
> >> >   ruby: bind database close()/destroy() properly
> >> 
> >> I agree with the discussion on IRC that the change in the first patch
> >> makes sense.
> >
> > Shall I push it to the master branch then?
> 
> sure.

Done.

> > I don't really care that much about patch #2, but #3 should probably be
> > applied.
> 
> Just to be clear, I wasn't objecting to patch 2, just asking for a few
> more words of commit message.

Yes, that's what I understood, but at the moment I don't thin it's worth my
time to do that. There's mor interesting stuff to do, maybe later.

> For patch 3, since it's ruby specific and
> nobody complained, I'd say go for it.

All right. I'll probably wait a bit more, maybe a week to see if somebody
shouts.

-- 
Felipe Contreras

Re: [PATCH 0/3] General fixes

[Tomi Ollila]

On Sat, Nov 02 2013, Felipe Contreras <felipe.contreras@gmail.com> wrote:

> David Bremner wrote:
>> Felipe Contreras <felipe.contreras@gmail.com> writes:
>> 
>> > David Bremner wrote:
>> >> Felipe Contreras <felipe.contreras@gmail.com> writes:
>> >> 
>> >> > Felipe Contreras (3):
>> >> >   query: bind queries to database objects
>> >> >   ruby: allow build with RUNPATH
>> >> >   ruby: bind database close()/destroy() properly
>> >> 
>> >> I agree with the discussion on IRC that the change in the first patch
>> >> makes sense.
>> >
>> > Shall I push it to the master branch then?
>> 
>> sure.
>
> Done.
>
>> > I don't really care that much about patch #2, but #3 should probably be
>> > applied.
>> 
>> Just to be clear, I wasn't objecting to patch 2, just asking for a few
>> more words of commit message.
>
> Yes, that's what I understood, but at the moment I don't thin it's worth my
> time to do that. There's mor interesting stuff to do, maybe later.

Lame excuse >;)

>
>> For patch 3, since it's ruby specific and
>> nobody complained, I'd say go for it.
>
> All right. I'll probably wait a bit more, maybe a week to see if somebody
> shouts.

Remember Feature freeze: November 8

>
> -- 
> Felipe Contreras

Tomi