Re: [Tracker] Merging Turtle branch to Trunk



On 10/12/08 15:37, Philip Van Hoof wrote:
This is today's patch which includes Ivan's most recent changes

svn merge -r 2555:2615 ../branches/turtle/<- needed conflict solving
svn merge -r 2615:2680 ../branches/turtle/<- needed conflict solving
svn merge -r 2680:2682 ../branches/turtle
svn merge -r 2682:2687 ../branches/turtle
svn merge -r 2687:2688 ../branches/turtle


Hi :)

Thanks for the update. My comments are below:


#1, Didn't we decide to depend on Raptor? If so, we don't need the AC_DEFINE right?

+# Check for Raptor
+PKG_CHECK_MODULES(RAPTOR, [raptor], have_raptor=yes, have_raptor=no)
+AC_SUBST(RAPTOR_CFLAGS)
+AC_SUBST(RAPTOR_LIBS)
+
+if test x$have_raptor == "xyes"; then
+  AC_DEFINE(HAVE_RAPTOR, 1, [Raptor RDF parsers])
+fi


#2. This definitely looks like a leak to me, since the "break" will leave the while() loop and the volume is freed ONLY in the opposite condition. Or am I missing something here?

+       while (g_hash_table_iter_next (&iter, &key, &value)) {
+               LibHalVolume  *volume;
+               const gchar   *udi;
+               const gchar   *mp;
+
+               udi = key;
+
+               volume = libhal_volume_from_udi (priv->context, udi);
+
+               if (!volume) {
+                       g_message ("HAL device with udi:'%s' has no volume, "
+                                  "should we delete?",
+                                  udi);
+                       continue;
+               }
+
+               mp = libhal_volume_get_mount_point (volume);
+
+               if (g_strcmp0 (mp, path) != 0) {
+                       if (g_strrstr (path, mp)) {
+                               found = TRUE;
+
+                               if (mount_point)
+                                       *mount_point = g_strdup (mp);
+
+                               if (available)
+                                       *available = libhal_volume_is_mounted (volume);
+                               break;
+                       }
+               }
+
+               libhal_volume_free (volume);


#3. We are checking the same thing twice here:

        field = tracker_ontology_get_field_by_name (field_name);

+       if (!field) {
+ g_warning ("Field name '%s' has isn't described in the ontology", field_name);
+               return;
+       }
+
+       field = tracker_ontology_get_field_by_name (field_name);
+
        g_return_if_fail (TRACKER_IS_FIELD (field));
        g_return_if_fail (tracker_field_get_multiple_values (field) == TRUE);


#4. I still don't like the Config and Language being static here. We already initialise the data module with them in tracker_data_manager_init (), why not just call tracker_data_manager_get_config() and tracker_data_manager_get_language() respectively?

+typedef struct {
+       TrackerService *service;
+       guint32 iid_value;
+       TrackerLanguage *language;
+       TrackerConfig *config;
+} ForeachInMetadataInfo;
+
+static TrackerConfig   *config = NULL;
+static TrackerLanguage *language = NULL;
+


#5. Shouldn't we be adding the Service type == Folder check here before doing anything recursive?

+       if (service_id != 0) {
+               tracker_data_update_delete_service (service, service_id);
+               tracker_data_update_delete_service_recursively (service, (gchar *) path);
+               tracker_data_update_delete_all_metadata (service, service_id);
+       }


#6. Extra spaces here, there are further down in the patch for this file too:

+               GList *list;
+
+               for (list = value; list; list = list->next)
+                       set_metadata (field, list->data, user_data);
+
+       }


#7. Can we remove these preprocessor operatives? and/or the debug statement itself? If not, we can always improve the debugging to actually mean something in the context of all the other logging.

+#ifdef TURTLE_DEBUG
+               g_debug ("Q ERROR: %s\n", error->message);
+#endif /* TURTLE_DEBUG */


#8. Since we decided to depend on Raptor, we can remove the #ifdefs in the tracker-turtle.[ch] I think. There are also a lot of code block blank lines which are not necessary.


#9. Include order should be system, highest dependency -> lowest dependency. i.e. these should be the opposite way round in the tracker-turtle.h:

+#include <libtracker-data/tracker-data-metadata.h>
+#include <stdio.h>


#10. Coding style, indentation, and again, stmt isn't a good name for a public typedef struct, can we call it something sensible like TrackerTurtleStatement? We shouldn't need the #idefs there too if we depend on Raptor.

+typedef struct {
+  gconstpointer subject;
+  gint subject_type;
+  gconstpointer predicate;
+  gint predicate_type;
+  gconstpointer object;
+  gint object_type;
+  gpointer object_literal_datatype;
+  const guchar *object_literal_language;
+} stmt;
+#endif


#11. Can we avoid comments in line like this, it looks ugly, it is better to have it above the function or not at all.

+void        tracker_turtle_add_metadatas   (TurtleFile          *turtle,
+                                           GPtrArray /* <TrackerTurtleMetadataItem> */ *metadata_items);


#12. There are some serious coding style issues in tracker-removable-device.c. We could probably also use g_strcmp0() in a lot of places to reduce the code down a bit and make it easier to read. That's really not completely necessary though for this patch to get committed.


#13. Is this a leak, looks like only str is freed, not service_type in the if (!field):

+               gchar *str = NULL;
+               gchar *uri;
+               gchar *service_type;
+
+               g_value_init (&transform, G_TYPE_STRING);
+
+               tracker_db_result_set_get (result_set, 0, &uri, -1);
+               tracker_db_result_set_get (result_set, 1, &service_type, -1);
+               tracker_db_result_set_get (result_set, 2, &metadata_id, -1);
+               tracker_db_result_set_get (result_set, 3, &str, -1);
+
+               field = tracker_ontology_get_field_by_id (metadata_id);
+               if (!field) {
+                       g_critical ("Field id %d in database but not in tracker-ontology",
+                                   metadata_id);
+                       g_free (str);
+                       return;
+               }


#14. There is no need to cast here in trackerd/tracker-main.c for callback_id:


+       gulong *callback_id = (gulong *)user_data;
+       GError *error;
+       static gint counter = 0;


#15. The || condition should be on the right here, also, this whole code block would be nicer in another function I think - which I commented on in my previous mail.

+       if (flags & TRACKER_DB_MANAGER_FORCE_REINDEX
+           || g_file_test (get_ttl_backup_filename, G_FILE_TEST_EXISTS)) {


Other than that, good work guys. I think we should try to get this commit this week!

--
Regards,
Martyn



[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]