On Di, 2011-11-29 at 21:53 +0100, Chris Kühl wrote:
> On Tue, Nov 29, 2011 at 5:56 PM, Patrick Ohly <[email protected]> wrote:
> > On Di, 2011-11-29 at 15:09 +0100, Chris Kühl wrote:
> >> On Tue, Nov 29, 2011 at 2:31 PM, Patrick Ohly <[email protected]> 
> >> wrote:
> >> > On Di, 2011-11-29 at 13:20 +0100, Chris Kühl wrote:
> > [patch with NULL check]
> >> > Does that look right?
> >> >
> >>
> >> Seems fine to me.
> >
> > I've merged your branch and applied the fix.
> >
> >> > Speaking of DBusErrorCXX, why does it have a separate "message" member?
> >> >
> >>
> >> That's only in there to maintain compatibility. I believe the examples
> >> were the only place the message field was used however. We could
> >> easliy change those and get read of this member.
> >
> > Yes, let's do that. I'm working on a patch.
> 
> Ok.

Done:

commit 7809e2488a9803ccce203566ff731c66b88da0d1
Author: Patrick Ohly <[email protected]>
Date:   Tue Nov 29 17:05:09 2011 +0000

    GIO GDBus: remove redundant pointer in DBusErrorCXX
    
    The "message" pointer merely mirrored m_error->message.
    A set(NULL) left a dangling "message" pointer behind.
    Therefore this patch removes the member and replaces it
    with the getMessage() method. Was only needed by an
    example program anyway.

I've also fixed the link error:

commit 8fc4a378ba99e7e66aceae097242ac0384d97a75
Author: Patrick Ohly <[email protected]>
Date:   Wed Nov 30 08:02:08 2011 +0000

    GIO GDBus: fixed link error
    
    Previously, DBUS_LIBS contained the dbus-1 libs required to link
    executables. When enabling GIO GDBus, GIO_DBUS_LIBS was set instead,
    but not used by most of the link rules (except for the new
    example). This caused link failures when using dynamic linking on
    recent Linux distros where all libraries used by an object file have
    to be listed explicitly - indirectly through some other shared object
    is not enough anymore.
    
    To solve this with minimal changes, DBUS_LIBS and DBUS_CFLAGS are
    reinterpreted as "the libs resp. flags needed for using D-Bus" and now
    get set for both dbus-1 and gio.

> > Looking closer at compiler output, I saw a warning about this code:
> >
> > class DBusResult : virtual public Result
> > {
> > [...]
> >    virtual void failed(const dbus_error &error)
> >    {
> >        GDBusMessage *errMsg;
> >        errMsg = g_dbus_message_new_method_error(m_msg.get(), 
> > error.dbusName().c_str(),
> >                                                 "%s", error.what());
> > ===>    if (!g_dbus_connection_send_message(m_conn.get(), m_msg.get(),
> >                                            G_DBUS_SEND_MESSAGE_FLAGS_NONE, 
> > NULL, NULL)) {
> >            throw std::runtime_error(" g_dbus_connection_send_message 
> > failed");
> >        }
> >    }
> >
> > gdbus-cxx-bridge.h: In member function 'virtual void 
> > GDBusCXX::DBusResult::failed(const GDBusCXX::dbus_error&)':
> > gdbus-cxx-bridge.h:1778:23: warning: variable 'errMsg' set but not used 
> > [-Wunused-but-set-variable]
> >
> > Is errMsg the message which is meant to be sent above?
> >
> 
> Yes. Thanks for catching that.

Fixed:

commit 1095d914029c184e1abe442d8a28c3a37073b2d5
Author: Patrick Ohly <[email protected]>
Date:   Wed Nov 30 08:40:57 2011 +0000

    GIO GDBus: fixed cut-and-paste error in error handling
    
    DBusResult::failed() sent the wrong message due to a cut-and-paste error.
    Found with gcc 4.6 and -Wall, because errMsg was set but not used.


It's interesting that this wasn't caught as a fatal error by the
testing. Ah, multiple reasons for that:
      * Testing disabled that check in most cases in the configuration
        of the platform with -Wno-unused-but-set-variable in CFLAGS.
      * Even if that hadn't been the case, it was also set for the
        --enable-warnings settings in configure.

Testing now uses --enable-warnings=fatal/max consistently (fatal in the
compile tests, max when producing binaries for the testing), and
-Wno-unused-but-set-variable is no longer set because it hides genuine
problems.

It used to be set for libsynthesis because the code there has too many
valid reasons for setting and not using something (mostly due to ifdefs
and macros). But SyncEvolution should be clean to pass with that check,
and with --enable-warnings it is possible to configure SyncEvolution and
libsynthesis differently.


-- 
Best Regards, Patrick Ohly

The content of this message is my personal opinion only and although
I am an employee of Intel, the statements I make here in no way
represent Intel's position on the issue, nor am I authorized to speak
on behalf of Intel on this matter.


_______________________________________________
SyncEvolution mailing list
[email protected]
http://lists.syncevolution.org/listinfo/syncevolution

Reply via email to