On Mon, 2010-02-22 at 13:52 +0000, Martyn Russell wrote:
> Hi all,
>
> Philip recently worked on the ontology-cope-for-master branch to add
> support for real time ontology updates (usually by 3rd party apps that
> are installed for example). This is the review of that branch:
>
> http://git.gnome.org/browse/tracker/log/?h=ontology-cope-for-master
>
> Warning, pedandic comments follow :)
:-)
> --
>
> 1. Can we avoid using "Priv" and use "Private" for TrackerOntologyPriv
> et al, we use "Private" everywhere else. If we don't in some other place
> then that place is wrong too.
Fixed
> 2. tracker_ontology_get_last_modified() returns a time_t but you cast to
> a gint64 before returning the value, shouldn't we cast to the return
> type OR change the return type?
Fixed, no casting was needed at all here
> 3. Just a note, in get_ontology_from_file() you do this:
>
> + TrackerTurtleReader *reader;
> + GError *error = NULL;
> + GHashTable *ontology_uris;
> + TrackerOntology *ret = NULL;
>
> Variable alignments are not necessary any more.
Okay
> 4. You reference the ontology then unreference it in the next statement
> in get_ontology_from_file():
>
> + g_hash_table_insert (ontology_uris,
> + g_strdup (subject),
> + g_object_ref (ontology));
> +
> + g_object_unref (ontology);
>
> Why not save the extra CPU cycles and pass ontology without ref/unref there?
Fixed
> 5. Just a question really, but I wonder if we should have some debugging
> in get_ontology_from_file() so we know what file we are parsing and what
> we are doing during that phase?
No, this is done for each ontology file at startup, yet the file isn't
processed (notice the break): it's only to get the nao:lastModified date
for comparison later.
> 6. In get_ontology_from_file() I notice we return a referenced
> TrackerOntology, it would probably be more efficient to return the
> pointer in the hash table instead to save the additional ref/unref.
ok (fine for me)
> 7. In get_ontologies() it is faster to sort the list AFTER prepending
> the items than each time:
>
> + sorted = g_list_insert_sorted (sorted,
> + g_strdup (conf_file),
> + (GCompareFunc) strcmp);
>
> Probably not noticeable here though and I notice it is just moved code
> that was there already anyway :)
Yes, I prefer not to change this code because it alters existing code
while this code moving was done for reusing said code. Such a change
(sorting after prepending) wouldn't have anything to do with the feature
being implemented here.
> 8. We don't generally use \t in debugging because the indentation is too
> much, 2 spaces is normally what's used, also, please use a parameter per
> line, it is clearer to see what is going on when you have a bunch of
> string manipulation going on.
>
> + g_debug ("%sAltering database for class '%s' property '%s': single
> value (%s)",
> + in_alter ? "" : "\t", service_name, field_name,
> + in_alter ? "alter" : "create");
Fixed
>
> 9. Does this mean we only support NEW ontologies, not UPDATES?
>
> + /* TODO: copy with tracker_property_get_is_new together with a
> + * tracker_property_get_multiple_values (ALTER TABLE situation) */
This comment is removed in a later commit
> 10. Don't use tracker_ namespace for internal functions, coding style:
>
> + static gint
> + tracker_data_update_get_new_service_id (void)
This change was reverted in a later commit, and thus not related to the
branch as a whole.
> There are others in that file too of course, but they should be fixed
> another time.
> --
>
> Really good work here Philip, this is not an easy bit of work to review
> or implement so kudos to you for doing this!
No problem :)
--
Philip Van Hoof, freelance software developer
home: me at pvanhoof dot be
gnome: pvanhoof at gnome dot org
http://pvanhoof.be/blog
http://codeminded.be
_______________________________________________
tracker-list mailing list
[email protected]
http://mail.gnome.org/mailman/listinfo/tracker-list
