Code review comment for lp:~ted/indicator-appmenu/hud

Revision history for this message
Allan LeSage (allanlesage) wrote :

For usage-tracker.c:

Ted did you consider just up-ticking an entry-count, e.g., when it's marked, and then returning the count when found? I assume this complicates your expiration mechanism. I'm told that the count operation is pretty quick so maybe that's fine.

I'm assuming it's no coincidence that you gave me a file to review which has high test-coverage :) . In configure_db if we get an "Error building LRU DB", what happens, and do we really want to continue? I see that these are the non-tested paths.

I should say that I don't feel confident enough yet in gobject to give a larsu- or awe-style critique of your C--I'm informing you so that you can refer this review again as necessary; I assume you'll apply their advice here.

Mikkel's advice to add an index is good advice IMHO but I wonder how much benefit you'd see as this is a dirt-simple db; it would good to add some performance testing for this, e.g.

All the GoogleTest work is really excellent to see. This is barely worth a commit but consider putting the asserted val first in an assertion.

On line 362 I'm seeing what might be a 'TODO', but I'm not sure how you'd load existing data on a fresh build--what does this mean, and do you want to address?

I'm eager to get into the performance testing and filling in the gaps for test; I think items 2 and 6 above are worth addressing for an approval.

review: Needs Information

« Back to merge proposal