Created attachment 364750 [details] [review]
Drop unnecessary packed attribute

There is no need to pack a structure with four uniform types, and the
compiler warns about it if you use -Wpacked — which is the default when
using AX_COMPILER_FLAGS.

I've added attachment 364750 [details] [review] to Continuous for the time being.

(In reply to Emmanuele Bassi (:ebassi) from comment #0)
> The `packed` attribute for TrackerResourcesEvent is not needed: the
> structure is made of four identical types, and the compiler will be able
> to pack it by itself.

So, later on in the same file, we have:

events
  = (const TrackerResourcesEvent *)
      g_variant_get_fixed_array (delete_events,
                                 &n_elements,                                                          
                                 sizeof (TrackerResourcesEvent));

...

events
  = (const TrackerResourcesEvent *)
      g_variant_get_fixed_array (insert_events,
                                 &n_elements,                                                          
                                 sizeof (TrackerResourcesEvent));

... in the handler for this D-Bus signal:

<interface name="org.freedesktop.Tracker1.Resources">
  <signal name="GraphUpdated">
    <arg name="className" type="s" />
    <arg name="deleteEvents" type="a(iiii)" />
    <arg name="insertEvents" type="a(iiii)" />
  </signal>
</interface>

I wanted to ensure that a struct with four gint32 members would exactly match the memory layout of a(iiii) on all architectures.

The same code in JS, as used in gnome-documents, looks like:

deleteEvents.forEach(Lang.bind(this,
    function(event) {
        this._addPendingEvent(event, true);
    }));

insertEvents.forEach(Lang.bind(this,
    function(event) {
        this._addPendingEvent(event, false);
    }));

But I don't understand JS enough to know how that gets translated into the JS object "event".

There is an example using raw GDBus (ie. without a gdbus-codegen:ed proxy):
https://wiki.gnome.org/Projects/Tracker/Documentation/SignalsOnChanges

Long story short, I never found the time to read through the gdbus-codegen:ed code, GVariant and D-Bus to convince myself whether the packing is really necessary or not.

Obviously, this GraphUpdated signal for change notification is one of the dark corners of the Tracker API, and these days there is TrackerNotifier. So the whole PhotosTrackerChangeMonitor class should be replaced with that (bug 776671). And that's another thing that I didn't get to yet.

Review of attachment 364750 [details] [review]:

Thanks for verifying that the packing is indeed not necessary. Pushed to master.

I am curious how/why it broke Continuous.

The initial version of AX_COMPILER_FLAGS that we started with never broke the build even though it flagged the -Wpacked, and was supposed to use -Werror for non-tarball builds. Then at some point I tried to bump the macros, it broke, and reverted back to the earlier version.

commit ac6d779679f320fa7cb3e5876d21d745c9d070b8
Author: Debarshi Ray <debarshir@gnome.org>
Date:   Thu Jun 8 09:57:31 2017 +0200

    Revert "build: Update all AX_* macros from upstream"
    
    This seems to have broken Continuous with:
      ../../src/photos-tracker-change-monitor.c:95:1: error: packed
        attribute is unnecessary for '_TrackerResourcesEvent'
        [-Werror=packed] } __attribute__ ((packed));
    
    This warning and our use of -Wpacked aren't new. They have been present
    for a while. It is not clear to me how Continuous managed to work so
    far, and why it broke only now. I am also failing to spot any relevant
    change in the macros that might have caused this.
    
    This reverts commit 548c27eb80f806e94adfbf6061b897518f650aa4.

Since the warning was going to go away, one or another (see comment 3), and with Meson on the horizon, I didn't invest much time in figuring out what was wrong with AX_COMPILER_FLAGS so far.

The Flatpak and jhbuild has so far worked by virtue of --disable-Werror.

(While I like AX_COMPILER_FLAGS's choice of flags, I don't quite like -Werror even in development builds. With different builds using different toolchains, it seems a bit too aggressive.)

Anyway, thank you once again for verifying that the packing was indeed not necessary.