After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 786700 - Code Search for Builder
Code Search for Builder
Status: RESOLVED FIXED
Product: gnome-builder
Classification: Other
Component: search
3.25.x
Other Linux
: Normal normal
: ---
Assigned To: GNOME Builder Maintainers
GNOME Builder Maintainers
Depends on:
Blocks:
 
 
Reported: 2017-08-23 19:43 UTC by Anoop Chandu
Modified: 2017-10-16 08:58 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
code-index: Adding IdeCodeIndexService to code-index (23.62 KB, patch)
2017-08-23 20:52 UTC, Anoop Chandu
none Details | Review
symbols: Added symbols, flags and icons (3.62 KB, patch)
2017-08-23 20:53 UTC, Anoop Chandu
committed Details | Review
symbols: Adding IdeCodeIndexer,IdeCodeIndexEntry interfaces (16.75 KB, patch)
2017-08-23 20:53 UTC, Anoop Chandu
none Details | Review
Adding persistent map (26.82 KB, patch)
2017-08-23 20:53 UTC, Anoop Chandu
none Details | Review
code-index: Adding IdeCodeIndexBuilder (33.33 KB, patch)
2017-08-23 20:54 UTC, Anoop Chandu
none Details | Review
code-index: Adding IdeCodeIndexIndex (24.35 KB, patch)
2017-08-23 20:54 UTC, Anoop Chandu
none Details | Review
clang: Implement IdeCodeIndexer (14.65 KB, patch)
2017-08-23 20:55 UTC, Anoop Chandu
none Details | Review
clang: Implement GListModel, IdeCodeIndexEntry (24.29 KB, patch)
2017-08-23 20:55 UTC, Anoop Chandu
none Details | Review
code-index: Implementing IdeSearchResult (7.80 KB, patch)
2017-08-23 20:56 UTC, Anoop Chandu
none Details | Review
code-index: Implmenting search provider (6.25 KB, patch)
2017-08-23 20:56 UTC, Anoop Chandu
none Details | Review
code-index: Implementing symbol resolver (7.54 KB, patch)
2017-08-23 20:57 UTC, Anoop Chandu
none Details | Review
buffer: Use multiple symbol resolvers instead of one. (48.31 KB, patch)
2017-08-23 20:57 UTC, Anoop Chandu
none Details | Review
code-index: Adding plugin files (5.05 KB, patch)
2017-08-23 20:58 UTC, Anoop Chandu
committed Details | Review
clang: Adding code indexer (2.25 KB, patch)
2017-08-23 20:58 UTC, Anoop Chandu
none Details | Review
buildsystem: Handle empty file array and cancellation of task (2.83 KB, patch)
2017-08-23 20:59 UTC, Anoop Chandu
none Details | Review
libide: Unload services before destroying RuntimeManager (2.07 KB, patch)
2017-08-23 20:59 UTC, Anoop Chandu
none Details | Review
code-index: Removing some messages and cleaning code (13.60 KB, patch)
2017-08-23 21:00 UTC, Anoop Chandu
none Details | Review
code-index: Adding IdeCodeIndexService to code-index (16.82 KB, patch)
2017-08-26 15:58 UTC, Anoop Chandu
none Details | Review
symbols: Adding IdeCodeIndexer,IdeCodeIndexEntry interfaces (19.32 KB, patch)
2017-08-26 15:59 UTC, Anoop Chandu
none Details | Review
Adding persistent map (28.62 KB, patch)
2017-08-26 16:00 UTC, Anoop Chandu
committed Details | Review
code-index: Adding IdeCodeIndexBuilder (33.53 KB, patch)
2017-08-26 16:01 UTC, Anoop Chandu
none Details | Review
code-index: Adding IdeCodeIndexIndex (24.25 KB, patch)
2017-08-26 16:02 UTC, Anoop Chandu
committed Details | Review
clang: Implement IdeCodeIndexer (14.86 KB, patch)
2017-08-26 16:03 UTC, Anoop Chandu
committed Details | Review
code-index: Implementing IdeSearchResult (7.81 KB, patch)
2017-08-26 16:04 UTC, Anoop Chandu
committed Details | Review
code-index: Implmenting search provider (6.27 KB, patch)
2017-08-26 16:04 UTC, Anoop Chandu
committed Details | Review
code-index: Implementing symbol resolver (7.56 KB, patch)
2017-08-26 16:05 UTC, Anoop Chandu
committed Details | Review
buffer: Use multiple symbol resolvers instead of one. (48.57 KB, patch)
2017-08-26 16:06 UTC, Anoop Chandu
none Details | Review
buildsystem: Handle empty file array and task cancellation (2.55 KB, patch)
2017-08-26 16:07 UTC, Anoop Chandu
committed Details | Review
libide: Unload services before destroying RuntimeManager (2.06 KB, patch)
2017-08-26 16:09 UTC, Anoop Chandu
accepted-commit_now Details | Review
symbols: Adding IdeCodeIndexEntries interface (4.12 KB, patch)
2017-08-26 16:10 UTC, Anoop Chandu
none Details | Review
clang: Implement IdeCodeIndexEntries and IdeCodeIndexEntry (23.63 KB, patch)
2017-08-26 16:12 UTC, Anoop Chandu
none Details | Review
code-index: fixes in IdeCodeIndexBuilder and IdePersistentMap (2.62 KB, patch)
2017-08-27 06:02 UTC, Anoop Chandu
accepted-commit_now Details | Review
symbols: Adding IdeCodeIndexEntries interface (4.12 KB, patch)
2017-08-29 16:02 UTC, Anoop Chandu
committed Details | Review
symbols: Adding IdeCodeIndexer & IdeCodeIndexEntry (22.72 KB, patch)
2017-08-29 16:04 UTC, Anoop Chandu
committed Details | Review
clang: Implement IdeCodeIndexEntries (15.29 KB, patch)
2017-08-29 16:06 UTC, Anoop Chandu
committed Details | Review
code-index: Adding IdeCodeIndexBuilder (33.53 KB, patch)
2017-08-29 16:07 UTC, Anoop Chandu
committed Details | Review
code-index: Adding IdeCodeIndexService to code-index (16.86 KB, patch)
2017-08-29 16:09 UTC, Anoop Chandu
committed Details | Review
clang: Adding code indexer (2.18 KB, patch)
2017-08-29 16:10 UTC, Anoop Chandu
committed Details | Review
buffer: Use multiple symbol resolvers instead of one. (48.43 KB, patch)
2017-08-29 16:13 UTC, Anoop Chandu
committed Details | Review
code-index: fix infinte loop in binary search (1015 bytes, patch)
2017-09-01 22:23 UTC, Christian Hergert
committed Details | Review
code-index: Avoid infinite build loop (5.22 KB, patch)
2017-09-02 11:31 UTC, Anoop Chandu
committed Details | Review
code-index: Avoid segfault when indexing is stopped (1.90 KB, patch)
2017-09-02 13:00 UTC, Anoop Chandu
accepted-commit_now Details | Review

Description Anoop Chandu 2017-08-23 19:43:52 UTC
This is for tracking GSOC Project Code Search for GNOME Builder,https://summerofcode.withgoogle.com/projects/#4574885478137856. Continuation of https://bugzilla.gnome.org/show_bug.cgi?id=783764.
Comment 1 Anoop Chandu 2017-08-23 20:52:31 UTC
Created attachment 358271 [details] [review]
code-index: Adding IdeCodeIndexService to code-index
Comment 2 Anoop Chandu 2017-08-23 20:53:02 UTC
Created attachment 358272 [details] [review]
symbols: Added symbols, flags and icons
Comment 3 Anoop Chandu 2017-08-23 20:53:30 UTC
Created attachment 358273 [details] [review]
symbols: Adding IdeCodeIndexer,IdeCodeIndexEntry  interfaces
Comment 4 Anoop Chandu 2017-08-23 20:53:57 UTC
Created attachment 358274 [details] [review]
Adding persistent map
Comment 5 Anoop Chandu 2017-08-23 20:54:21 UTC
Created attachment 358275 [details] [review]
code-index: Adding IdeCodeIndexBuilder
Comment 6 Anoop Chandu 2017-08-23 20:54:48 UTC
Created attachment 358276 [details] [review]
code-index: Adding IdeCodeIndexIndex
Comment 7 Anoop Chandu 2017-08-23 20:55:16 UTC
Created attachment 358278 [details] [review]
clang: Implement IdeCodeIndexer
Comment 8 Anoop Chandu 2017-08-23 20:55:55 UTC
Created attachment 358279 [details] [review]
clang: Implement GListModel, IdeCodeIndexEntry
Comment 9 Anoop Chandu 2017-08-23 20:56:14 UTC
Created attachment 358280 [details] [review]
code-index: Implementing IdeSearchResult
Comment 10 Anoop Chandu 2017-08-23 20:56:49 UTC
Created attachment 358281 [details] [review]
code-index: Implmenting search provider
Comment 11 Anoop Chandu 2017-08-23 20:57:14 UTC
Created attachment 358282 [details] [review]
code-index: Implementing symbol resolver
Comment 12 Anoop Chandu 2017-08-23 20:57:48 UTC
Created attachment 358283 [details] [review]
buffer: Use multiple symbol resolvers instead of one.
Comment 13 Anoop Chandu 2017-08-23 20:58:18 UTC
Created attachment 358284 [details] [review]
code-index: Adding plugin files
Comment 14 Anoop Chandu 2017-08-23 20:58:53 UTC
Created attachment 358285 [details] [review]
clang: Adding code indexer
Comment 15 Anoop Chandu 2017-08-23 20:59:17 UTC
Created attachment 358286 [details] [review]
buildsystem: Handle empty file array and cancellation  of task
Comment 16 Anoop Chandu 2017-08-23 20:59:38 UTC
Created attachment 358287 [details] [review]
libide: Unload services before destroying  RuntimeManager
Comment 17 Anoop Chandu 2017-08-23 21:00:06 UTC
Created attachment 358288 [details] [review]
code-index: Removing some messages and cleaning code
Comment 18 Christian Hergert 2017-08-26 00:24:12 UTC
Review of attachment 358271 [details] [review]:

Couple leaks to fix. If I manage to get to it today, I might just merge + fixup afterwards, but time will tell.

::: plugins/code-index/ide-code-index-service.c
@@ +73,3 @@
+
+static void
+build_data_free (BuildData *data,

No need to have the user_data parameter, it's fine to cast a (GDestroyNotify) to (GFunc) as long as you don't use user_data.

@@ +92,3 @@
+                                 gpointer      user_data)
+{
+  g_autoptr(IdeCodeIndexService) self = (IdeCodeIndexService *)user_data;

You don't need casts for gpointer/void* in C.

Also, this leaks self since it receives a reference when called. Use g_autoptr(IdeCodeIndexService) self = user_data; to fix it.

@@ +188,3 @@
+      bdata->recursive = recursive;
+
+      source_id = g_timeout_add_seconds (5, (GSourceFunc)ide_code_index_serivce_push, bdata);

Use a #define for something like DEFAULT_INDEX_TIMEOUT_SECS or something like that.

@@ +381,3 @@
+              g_hash_table_insert (self->code_indexers,
+                                   g_strdup (languages[i]),
+                                   g_object_ref (adapter));

g_steal_pointer (&adapter)

@@ +454,3 @@
+  IdeCodeIndexService *self = (IdeCodeIndexService *)service;
+
+  if (self->cancellable != NULL)

No need for the if check, g_cancellable_cancel() accepts NULL

@@ +497,3 @@
+}
+
+// static IdeCodeIndexer *

It would be nice to remove the unused code
Comment 19 Christian Hergert 2017-08-26 00:24:56 UTC
Review of attachment 358272 [details] [review]:

Cool, LGTM
Comment 20 Christian Hergert 2017-08-26 00:36:28 UTC
Review of attachment 358273 [details] [review]:

Looks good. If you feel like it, add "Since: 3.26" to the gtk-doc blocks since this is API that can be consumed by plugins.

::: libide/symbols/ide-code-index-entry.c
@@ +27,3 @@
+{
+}
+

We could use some documentation on these interface methods, since "USR" isn't a well known word outside of those that have used libclang.

::: libide/symbols/ide-code-index-entry.h
@@ +44,3 @@
+};
+
+/* make this const */

I think you can remove this comment, because in general we don't want to use "const strings" from interfaces as it makes things extra difficult on language bindings (which can leak strings in those scenarios). It's why most of our other interfaces are gchar*.

::: libide/symbols/ide-code-indexer.c
@@ +96,3 @@
+  IdeCodeIndexerInterface *iface;
+
+  g_return_val_if_fail (IDE_IS_CODE_INDEXER (self), NULL);

type check for file, and !cancellable || G_IS_CANCELLABLE (cancellable)

If build_flags can be NULL, then set (nullable) in the gtk-doc, otherwise NULL check.

@@ +107,3 @@
+ * @self: A #IdeCodeIndexer instance.
+ * @location: Source location of refernece.
+ * @cancellable: (allow-none): A #GCancellable.

allow-none is depreciated, use (nullable)

@@ +122,3 @@
+  IdeCodeIndexerInterface *iface;
+
+  g_return_if_fail (IDE_IS_CODE_INDEXER (self));

parameter checks location/cancellable too

::: libide/symbols/ide-code-indexer.h
@@ +52,3 @@
+                                               GCancellable         *cancellable,
+                                               GError              **error);
+void         ide_code_indexer_get_key_async   (IdeCodeIndexer       *self,

We should avoid "get" names for non-accessor functions. Maybe something like build/create/make/generate key.
Comment 21 Christian Hergert 2017-08-26 01:14:14 UTC
Review of attachment 358274 [details] [review]:

Looks good, but there are still some improvements we can make to ensure that this is in good shape to use for future projects.

::: plugins/code-index/ide-persistent-map-builder.c
@@ +84,3 @@
+
+      /*
+       * Key in hash table will point to actual key string in byte array.

This is confusing or inaccurate. It can't be in the byte array (as the byte array could be resized and moved in memory) and it it looks like it's strdup()'d anyway? If it has changed, the comment needs updating.

@@ +138,3 @@
+  g_variant_dict_init (&dict, NULL);
+
+  keys = g_variant_new_fixed_array ((const GVariantType *) "y",

G_VARIANT_TYPE_BYTE

@@ +144,3 @@
+
+  values = g_variant_new_array (NULL,
+                                (GVariant * const *)self->values->pdata,

You can possibly get cast warnings here due to alignment requirements, so do:  (GVariant * const *)(gpointer)self->values->pdata

@@ +148,3 @@
+
+  g_array_sort_with_data (self->kvpairs, (GCompareDataFunc)compare, self->keys->data);
+  kvpairs = g_variant_new_fixed_array ((const GVariantType *) "(uu)",

G_VARIANT_TYPE ("(uu)") which in addition to casting, does a format type check.

@@ +180,3 @@
+    g_task_return_boolean (task, TRUE);
+  else
+    g_task_return_error (task, error);

I want to see us be very explicit in the code-base about ownership transfers, especially for errors. I've fixed a number of bugs that would have been easier to find if they used g_autoptr(GError) error = NULL; and g_steal_pointer() when ownership transferred to the called function.

@@ +191,3 @@
+{
+  g_autoptr(GTask) task = NULL;
+

Always to precondition checks.

@@ +223,3 @@
+  g_task_set_source_tag (task, ide_persistent_map_builder_write_async);
+
+  ide_thread_pool_push_task (IDE_THREAD_POOL_INDEXER, task, ide_persistent_map_builder_write_worker);

No need to use the INDEXER thread pool for this operation, which is just an IO write really. Just use g_task_run_in_thread() which uses the default threadpool (primarily focused around doing I/O).

@@ +271,3 @@
+
+IdePersistentMapBuilder*
+ide_persistent_map_builder_new ()

(void)

::: plugins/code-index/ide-persistent-map.c
@@ +49,3 @@
+  gsize              n_kvpairs;
+
+  gboolean           loaded : 1;

gboolean loaded : 1 is incorrect as gboolean is a *signed* int (for historical reasons). Therefore, you only have a single bit, which is the signed bit. Use "guint loaded : 1;" to be correct here.

@@ +135,3 @@
+      return;
+    }
+

Let's add an endian check here, by looking up a new "byte-order" field in the dict. Set it to self->byte_order. Then in the lookup routine, we can g_variant_byteswap() if necessary.

@@ +212,3 @@
+  g_task_set_source_tag (task, ide_persistent_map_load_file_async);
+
+  ide_thread_pool_push_task (IDE_THREAD_POOL_INDEXER, task, ide_persistent_map_load_file_worker);

g_task_run_in_thread(), as this is just generic I/O.

@@ +227,3 @@
+}
+
+GVariant *

Missing a gtk-doc with (transfer full) for the return type.

Additionally, if we track the endianness of the creation (which defaults to host endianness), then we can return something that has g_variant_byteswap() so the consumer doesn't need to know about it.

@@ +238,3 @@
+
+  l = 0;
+  r = self->n_kvpairs - 1; /* unsigned long to signed long */

While this works with GCC/Clang, I'm not sure that this is not undefined behavior without a manual cast to (glong) before the subtraction.

(glong)self->n_kvpairs - 1

@@ +268,3 @@
+  g_return_val_if_fail (IDE_IS_PERSISTENT_MAP (self), 0);
+
+  value = g_variant_dict_lookup_value (self->metadata, key, G_VARIANT_TYPE_UINT64);

You can avoid creating the variant here with:

if (g_variant_dict_lookup (self->metadata, key, G_VARIANT_TYPE_UINT64, &v64))
  return v64;

@@ +304,3 @@
+
+IdePersistentMap*
+ide_persistent_map_new ()

(void)

::: plugins/code-index/ide-persistent-map.h
@@ +39,3 @@
+                                                                        GAsyncResult        *result,
+                                                                        GError             **error);
+GVariant               *ide_persistent_map_lookup_value                (IdePersistentMap    *self,

Mind the alignment
Comment 22 Christian Hergert 2017-08-26 01:15:37 UTC
Review of attachment 358274 [details] [review]:

Looks good, but there are still some improvements we can make to ensure that this is in good shape to use for future projects.

::: plugins/code-index/ide-persistent-map-builder.c
@@ +84,3 @@
+
+      /*
+       * Key in hash table will point to actual key string in byte array.

This is confusing or inaccurate. It can't be in the byte array (as the byte array could be resized and moved in memory) and it it looks like it's strdup()'d anyway? If it has changed, the comment needs updating.

@@ +138,3 @@
+  g_variant_dict_init (&dict, NULL);
+
+  keys = g_variant_new_fixed_array ((const GVariantType *) "y",

G_VARIANT_TYPE_BYTE

@@ +144,3 @@
+
+  values = g_variant_new_array (NULL,
+                                (GVariant * const *)self->values->pdata,

You can possibly get cast warnings here due to alignment requirements, so do:  (GVariant * const *)(gpointer)self->values->pdata

@@ +148,3 @@
+
+  g_array_sort_with_data (self->kvpairs, (GCompareDataFunc)compare, self->keys->data);
+  kvpairs = g_variant_new_fixed_array ((const GVariantType *) "(uu)",

G_VARIANT_TYPE ("(uu)") which in addition to casting, does a format type check.

@@ +180,3 @@
+    g_task_return_boolean (task, TRUE);
+  else
+    g_task_return_error (task, error);

I want to see us be very explicit in the code-base about ownership transfers, especially for errors. I've fixed a number of bugs that would have been easier to find if they used g_autoptr(GError) error = NULL; and g_steal_pointer() when ownership transferred to the called function.

@@ +191,3 @@
+{
+  g_autoptr(GTask) task = NULL;
+

Always to precondition checks.

@@ +223,3 @@
+  g_task_set_source_tag (task, ide_persistent_map_builder_write_async);
+
+  ide_thread_pool_push_task (IDE_THREAD_POOL_INDEXER, task, ide_persistent_map_builder_write_worker);

No need to use the INDEXER thread pool for this operation, which is just an IO write really. Just use g_task_run_in_thread() which uses the default threadpool (primarily focused around doing I/O).

@@ +271,3 @@
+
+IdePersistentMapBuilder*
+ide_persistent_map_builder_new ()

(void)

::: plugins/code-index/ide-persistent-map.c
@@ +49,3 @@
+  gsize              n_kvpairs;
+
+  gboolean           loaded : 1;

gboolean loaded : 1 is incorrect as gboolean is a *signed* int (for historical reasons). Therefore, you only have a single bit, which is the signed bit. Use "guint loaded : 1;" to be correct here.

@@ +135,3 @@
+      return;
+    }
+

Let's add an endian check here, by looking up a new "byte-order" field in the dict. Set it to self->byte_order. Then in the lookup routine, we can g_variant_byteswap() if necessary.

@@ +212,3 @@
+  g_task_set_source_tag (task, ide_persistent_map_load_file_async);
+
+  ide_thread_pool_push_task (IDE_THREAD_POOL_INDEXER, task, ide_persistent_map_load_file_worker);

g_task_run_in_thread(), as this is just generic I/O.

@@ +227,3 @@
+}
+
+GVariant *

Missing a gtk-doc with (transfer full) for the return type.

Additionally, if we track the endianness of the creation (which defaults to host endianness), then we can return something that has g_variant_byteswap() so the consumer doesn't need to know about it.

@@ +238,3 @@
+
+  l = 0;
+  r = self->n_kvpairs - 1; /* unsigned long to signed long */

While this works with GCC/Clang, I'm not sure that this is not undefined behavior without a manual cast to (glong) before the subtraction.

(glong)self->n_kvpairs - 1

@@ +268,3 @@
+  g_return_val_if_fail (IDE_IS_PERSISTENT_MAP (self), 0);
+
+  value = g_variant_dict_lookup_value (self->metadata, key, G_VARIANT_TYPE_UINT64);

You can avoid creating the variant here with:

if (g_variant_dict_lookup (self->metadata, key, G_VARIANT_TYPE_UINT64, &v64))
  return v64;

@@ +304,3 @@
+
+IdePersistentMap*
+ide_persistent_map_new ()

(void)

::: plugins/code-index/ide-persistent-map.h
@@ +39,3 @@
+                                                                        GAsyncResult        *result,
+                                                                        GError             **error);
+GVariant               *ide_persistent_map_lookup_value                (IdePersistentMap    *self,

Mind the alignment
Comment 23 Christian Hergert 2017-08-26 01:16:03 UTC
Review of attachment 358274 [details] [review]:

Looks good, but there are still some improvements we can make to ensure that this is in good shape to use for future projects.

::: plugins/code-index/ide-persistent-map-builder.c
@@ +84,3 @@
+
+      /*
+       * Key in hash table will point to actual key string in byte array.

This is confusing or inaccurate. It can't be in the byte array (as the byte array could be resized and moved in memory) and it it looks like it's strdup()'d anyway? If it has changed, the comment needs updating.

@@ +138,3 @@
+  g_variant_dict_init (&dict, NULL);
+
+  keys = g_variant_new_fixed_array ((const GVariantType *) "y",

G_VARIANT_TYPE_BYTE

@@ +144,3 @@
+
+  values = g_variant_new_array (NULL,
+                                (GVariant * const *)self->values->pdata,

You can possibly get cast warnings here due to alignment requirements, so do:  (GVariant * const *)(gpointer)self->values->pdata

@@ +148,3 @@
+
+  g_array_sort_with_data (self->kvpairs, (GCompareDataFunc)compare, self->keys->data);
+  kvpairs = g_variant_new_fixed_array ((const GVariantType *) "(uu)",

G_VARIANT_TYPE ("(uu)") which in addition to casting, does a format type check.

@@ +180,3 @@
+    g_task_return_boolean (task, TRUE);
+  else
+    g_task_return_error (task, error);

I want to see us be very explicit in the code-base about ownership transfers, especially for errors. I've fixed a number of bugs that would have been easier to find if they used g_autoptr(GError) error = NULL; and g_steal_pointer() when ownership transferred to the called function.

@@ +191,3 @@
+{
+  g_autoptr(GTask) task = NULL;
+

Always to precondition checks.

@@ +223,3 @@
+  g_task_set_source_tag (task, ide_persistent_map_builder_write_async);
+
+  ide_thread_pool_push_task (IDE_THREAD_POOL_INDEXER, task, ide_persistent_map_builder_write_worker);

No need to use the INDEXER thread pool for this operation, which is just an IO write really. Just use g_task_run_in_thread() which uses the default threadpool (primarily focused around doing I/O).

@@ +271,3 @@
+
+IdePersistentMapBuilder*
+ide_persistent_map_builder_new ()

(void)

::: plugins/code-index/ide-persistent-map.c
@@ +49,3 @@
+  gsize              n_kvpairs;
+
+  gboolean           loaded : 1;

gboolean loaded : 1 is incorrect as gboolean is a *signed* int (for historical reasons). Therefore, you only have a single bit, which is the signed bit. Use "guint loaded : 1;" to be correct here.

@@ +135,3 @@
+      return;
+    }
+

Let's add an endian check here, by looking up a new "byte-order" field in the dict. Set it to self->byte_order. Then in the lookup routine, we can g_variant_byteswap() if necessary.

@@ +212,3 @@
+  g_task_set_source_tag (task, ide_persistent_map_load_file_async);
+
+  ide_thread_pool_push_task (IDE_THREAD_POOL_INDEXER, task, ide_persistent_map_load_file_worker);

g_task_run_in_thread(), as this is just generic I/O.

@@ +227,3 @@
+}
+
+GVariant *

Missing a gtk-doc with (transfer full) for the return type.

Additionally, if we track the endianness of the creation (which defaults to host endianness), then we can return something that has g_variant_byteswap() so the consumer doesn't need to know about it.

@@ +238,3 @@
+
+  l = 0;
+  r = self->n_kvpairs - 1; /* unsigned long to signed long */

While this works with GCC/Clang, I'm not sure that this is not undefined behavior without a manual cast to (glong) before the subtraction.

(glong)self->n_kvpairs - 1

@@ +268,3 @@
+  g_return_val_if_fail (IDE_IS_PERSISTENT_MAP (self), 0);
+
+  value = g_variant_dict_lookup_value (self->metadata, key, G_VARIANT_TYPE_UINT64);

You can avoid creating the variant here with:

if (g_variant_dict_lookup (self->metadata, key, G_VARIANT_TYPE_UINT64, &v64))
  return v64;

@@ +304,3 @@
+
+IdePersistentMap*
+ide_persistent_map_new ()

(void)

::: plugins/code-index/ide-persistent-map.h
@@ +39,3 @@
+                                                                        GAsyncResult        *result,
+                                                                        GError             **error);
+GVariant               *ide_persistent_map_lookup_value                (IdePersistentMap    *self,

Mind the alignment
Comment 24 Christian Hergert 2017-08-26 01:16:03 UTC
Review of attachment 358274 [details] [review]:

Looks good, but there are still some improvements we can make to ensure that this is in good shape to use for future projects.

::: plugins/code-index/ide-persistent-map-builder.c
@@ +84,3 @@
+
+      /*
+       * Key in hash table will point to actual key string in byte array.

This is confusing or inaccurate. It can't be in the byte array (as the byte array could be resized and moved in memory) and it it looks like it's strdup()'d anyway? If it has changed, the comment needs updating.

@@ +138,3 @@
+  g_variant_dict_init (&dict, NULL);
+
+  keys = g_variant_new_fixed_array ((const GVariantType *) "y",

G_VARIANT_TYPE_BYTE

@@ +144,3 @@
+
+  values = g_variant_new_array (NULL,
+                                (GVariant * const *)self->values->pdata,

You can possibly get cast warnings here due to alignment requirements, so do:  (GVariant * const *)(gpointer)self->values->pdata

@@ +148,3 @@
+
+  g_array_sort_with_data (self->kvpairs, (GCompareDataFunc)compare, self->keys->data);
+  kvpairs = g_variant_new_fixed_array ((const GVariantType *) "(uu)",

G_VARIANT_TYPE ("(uu)") which in addition to casting, does a format type check.

@@ +180,3 @@
+    g_task_return_boolean (task, TRUE);
+  else
+    g_task_return_error (task, error);

I want to see us be very explicit in the code-base about ownership transfers, especially for errors. I've fixed a number of bugs that would have been easier to find if they used g_autoptr(GError) error = NULL; and g_steal_pointer() when ownership transferred to the called function.

@@ +191,3 @@
+{
+  g_autoptr(GTask) task = NULL;
+

Always to precondition checks.

@@ +223,3 @@
+  g_task_set_source_tag (task, ide_persistent_map_builder_write_async);
+
+  ide_thread_pool_push_task (IDE_THREAD_POOL_INDEXER, task, ide_persistent_map_builder_write_worker);

No need to use the INDEXER thread pool for this operation, which is just an IO write really. Just use g_task_run_in_thread() which uses the default threadpool (primarily focused around doing I/O).

@@ +271,3 @@
+
+IdePersistentMapBuilder*
+ide_persistent_map_builder_new ()

(void)

::: plugins/code-index/ide-persistent-map.c
@@ +49,3 @@
+  gsize              n_kvpairs;
+
+  gboolean           loaded : 1;

gboolean loaded : 1 is incorrect as gboolean is a *signed* int (for historical reasons). Therefore, you only have a single bit, which is the signed bit. Use "guint loaded : 1;" to be correct here.

@@ +135,3 @@
+      return;
+    }
+

Let's add an endian check here, by looking up a new "byte-order" field in the dict. Set it to self->byte_order. Then in the lookup routine, we can g_variant_byteswap() if necessary.

@@ +212,3 @@
+  g_task_set_source_tag (task, ide_persistent_map_load_file_async);
+
+  ide_thread_pool_push_task (IDE_THREAD_POOL_INDEXER, task, ide_persistent_map_load_file_worker);

g_task_run_in_thread(), as this is just generic I/O.

@@ +227,3 @@
+}
+
+GVariant *

Missing a gtk-doc with (transfer full) for the return type.

Additionally, if we track the endianness of the creation (which defaults to host endianness), then we can return something that has g_variant_byteswap() so the consumer doesn't need to know about it.

@@ +238,3 @@
+
+  l = 0;
+  r = self->n_kvpairs - 1; /* unsigned long to signed long */

While this works with GCC/Clang, I'm not sure that this is not undefined behavior without a manual cast to (glong) before the subtraction.

(glong)self->n_kvpairs - 1

@@ +268,3 @@
+  g_return_val_if_fail (IDE_IS_PERSISTENT_MAP (self), 0);
+
+  value = g_variant_dict_lookup_value (self->metadata, key, G_VARIANT_TYPE_UINT64);

You can avoid creating the variant here with:

if (g_variant_dict_lookup (self->metadata, key, G_VARIANT_TYPE_UINT64, &v64))
  return v64;

@@ +304,3 @@
+
+IdePersistentMap*
+ide_persistent_map_new ()

(void)

::: plugins/code-index/ide-persistent-map.h
@@ +39,3 @@
+                                                                        GAsyncResult        *result,
+                                                                        GError             **error);
+GVariant               *ide_persistent_map_lookup_value                (IdePersistentMap    *self,

Mind the alignment
Comment 25 Christian Hergert 2017-08-26 01:16:03 UTC
Review of attachment 358274 [details] [review]:

Looks good, but there are still some improvements we can make to ensure that this is in good shape to use for future projects.

::: plugins/code-index/ide-persistent-map-builder.c
@@ +84,3 @@
+
+      /*
+       * Key in hash table will point to actual key string in byte array.

This is confusing or inaccurate. It can't be in the byte array (as the byte array could be resized and moved in memory) and it it looks like it's strdup()'d anyway? If it has changed, the comment needs updating.

@@ +138,3 @@
+  g_variant_dict_init (&dict, NULL);
+
+  keys = g_variant_new_fixed_array ((const GVariantType *) "y",

G_VARIANT_TYPE_BYTE

@@ +144,3 @@
+
+  values = g_variant_new_array (NULL,
+                                (GVariant * const *)self->values->pdata,

You can possibly get cast warnings here due to alignment requirements, so do:  (GVariant * const *)(gpointer)self->values->pdata

@@ +148,3 @@
+
+  g_array_sort_with_data (self->kvpairs, (GCompareDataFunc)compare, self->keys->data);
+  kvpairs = g_variant_new_fixed_array ((const GVariantType *) "(uu)",

G_VARIANT_TYPE ("(uu)") which in addition to casting, does a format type check.

@@ +180,3 @@
+    g_task_return_boolean (task, TRUE);
+  else
+    g_task_return_error (task, error);

I want to see us be very explicit in the code-base about ownership transfers, especially for errors. I've fixed a number of bugs that would have been easier to find if they used g_autoptr(GError) error = NULL; and g_steal_pointer() when ownership transferred to the called function.

@@ +191,3 @@
+{
+  g_autoptr(GTask) task = NULL;
+

Always to precondition checks.

@@ +223,3 @@
+  g_task_set_source_tag (task, ide_persistent_map_builder_write_async);
+
+  ide_thread_pool_push_task (IDE_THREAD_POOL_INDEXER, task, ide_persistent_map_builder_write_worker);

No need to use the INDEXER thread pool for this operation, which is just an IO write really. Just use g_task_run_in_thread() which uses the default threadpool (primarily focused around doing I/O).

@@ +271,3 @@
+
+IdePersistentMapBuilder*
+ide_persistent_map_builder_new ()

(void)

::: plugins/code-index/ide-persistent-map.c
@@ +49,3 @@
+  gsize              n_kvpairs;
+
+  gboolean           loaded : 1;

gboolean loaded : 1 is incorrect as gboolean is a *signed* int (for historical reasons). Therefore, you only have a single bit, which is the signed bit. Use "guint loaded : 1;" to be correct here.

@@ +135,3 @@
+      return;
+    }
+

Let's add an endian check here, by looking up a new "byte-order" field in the dict. Set it to self->byte_order. Then in the lookup routine, we can g_variant_byteswap() if necessary.

@@ +212,3 @@
+  g_task_set_source_tag (task, ide_persistent_map_load_file_async);
+
+  ide_thread_pool_push_task (IDE_THREAD_POOL_INDEXER, task, ide_persistent_map_load_file_worker);

g_task_run_in_thread(), as this is just generic I/O.

@@ +227,3 @@
+}
+
+GVariant *

Missing a gtk-doc with (transfer full) for the return type.

Additionally, if we track the endianness of the creation (which defaults to host endianness), then we can return something that has g_variant_byteswap() so the consumer doesn't need to know about it.

@@ +238,3 @@
+
+  l = 0;
+  r = self->n_kvpairs - 1; /* unsigned long to signed long */

While this works with GCC/Clang, I'm not sure that this is not undefined behavior without a manual cast to (glong) before the subtraction.

(glong)self->n_kvpairs - 1

@@ +268,3 @@
+  g_return_val_if_fail (IDE_IS_PERSISTENT_MAP (self), 0);
+
+  value = g_variant_dict_lookup_value (self->metadata, key, G_VARIANT_TYPE_UINT64);

You can avoid creating the variant here with:

if (g_variant_dict_lookup (self->metadata, key, G_VARIANT_TYPE_UINT64, &v64))
  return v64;

@@ +304,3 @@
+
+IdePersistentMap*
+ide_persistent_map_new ()

(void)

::: plugins/code-index/ide-persistent-map.h
@@ +39,3 @@
+                                                                        GAsyncResult        *result,
+                                                                        GError             **error);
+GVariant               *ide_persistent_map_lookup_value                (IdePersistentMap    *self,

Mind the alignment
Comment 26 Christian Hergert 2017-08-26 01:16:03 UTC
Review of attachment 358274 [details] [review]:

Looks good, but there are still some improvements we can make to ensure that this is in good shape to use for future projects.

::: plugins/code-index/ide-persistent-map-builder.c
@@ +84,3 @@
+
+      /*
+       * Key in hash table will point to actual key string in byte array.

This is confusing or inaccurate. It can't be in the byte array (as the byte array could be resized and moved in memory) and it it looks like it's strdup()'d anyway? If it has changed, the comment needs updating.

@@ +138,3 @@
+  g_variant_dict_init (&dict, NULL);
+
+  keys = g_variant_new_fixed_array ((const GVariantType *) "y",

G_VARIANT_TYPE_BYTE

@@ +144,3 @@
+
+  values = g_variant_new_array (NULL,
+                                (GVariant * const *)self->values->pdata,

You can possibly get cast warnings here due to alignment requirements, so do:  (GVariant * const *)(gpointer)self->values->pdata

@@ +148,3 @@
+
+  g_array_sort_with_data (self->kvpairs, (GCompareDataFunc)compare, self->keys->data);
+  kvpairs = g_variant_new_fixed_array ((const GVariantType *) "(uu)",

G_VARIANT_TYPE ("(uu)") which in addition to casting, does a format type check.

@@ +180,3 @@
+    g_task_return_boolean (task, TRUE);
+  else
+    g_task_return_error (task, error);

I want to see us be very explicit in the code-base about ownership transfers, especially for errors. I've fixed a number of bugs that would have been easier to find if they used g_autoptr(GError) error = NULL; and g_steal_pointer() when ownership transferred to the called function.

@@ +191,3 @@
+{
+  g_autoptr(GTask) task = NULL;
+

Always to precondition checks.

@@ +223,3 @@
+  g_task_set_source_tag (task, ide_persistent_map_builder_write_async);
+
+  ide_thread_pool_push_task (IDE_THREAD_POOL_INDEXER, task, ide_persistent_map_builder_write_worker);

No need to use the INDEXER thread pool for this operation, which is just an IO write really. Just use g_task_run_in_thread() which uses the default threadpool (primarily focused around doing I/O).

@@ +271,3 @@
+
+IdePersistentMapBuilder*
+ide_persistent_map_builder_new ()

(void)

::: plugins/code-index/ide-persistent-map.c
@@ +49,3 @@
+  gsize              n_kvpairs;
+
+  gboolean           loaded : 1;

gboolean loaded : 1 is incorrect as gboolean is a *signed* int (for historical reasons). Therefore, you only have a single bit, which is the signed bit. Use "guint loaded : 1;" to be correct here.

@@ +135,3 @@
+      return;
+    }
+

Let's add an endian check here, by looking up a new "byte-order" field in the dict. Set it to self->byte_order. Then in the lookup routine, we can g_variant_byteswap() if necessary.

@@ +212,3 @@
+  g_task_set_source_tag (task, ide_persistent_map_load_file_async);
+
+  ide_thread_pool_push_task (IDE_THREAD_POOL_INDEXER, task, ide_persistent_map_load_file_worker);

g_task_run_in_thread(), as this is just generic I/O.

@@ +227,3 @@
+}
+
+GVariant *

Missing a gtk-doc with (transfer full) for the return type.

Additionally, if we track the endianness of the creation (which defaults to host endianness), then we can return something that has g_variant_byteswap() so the consumer doesn't need to know about it.

@@ +238,3 @@
+
+  l = 0;
+  r = self->n_kvpairs - 1; /* unsigned long to signed long */

While this works with GCC/Clang, I'm not sure that this is not undefined behavior without a manual cast to (glong) before the subtraction.

(glong)self->n_kvpairs - 1

@@ +268,3 @@
+  g_return_val_if_fail (IDE_IS_PERSISTENT_MAP (self), 0);
+
+  value = g_variant_dict_lookup_value (self->metadata, key, G_VARIANT_TYPE_UINT64);

You can avoid creating the variant here with:

if (g_variant_dict_lookup (self->metadata, key, G_VARIANT_TYPE_UINT64, &v64))
  return v64;

@@ +304,3 @@
+
+IdePersistentMap*
+ide_persistent_map_new ()

(void)

::: plugins/code-index/ide-persistent-map.h
@@ +39,3 @@
+                                                                        GAsyncResult        *result,
+                                                                        GError             **error);
+GVariant               *ide_persistent_map_lookup_value                (IdePersistentMap    *self,

Mind the alignment
Comment 27 Christian Hergert 2017-08-26 01:17:07 UTC
Sorry about the multiple copies. I blame bugzilla.
Comment 28 Christian Hergert 2017-08-26 01:38:59 UTC
Review of attachment 358275 [details] [review]:

::: plugins/code-index/ide-code-index-builder.c
@@ +80,3 @@
+
+static gboolean
+timeval_compare (GTimeVal a, GTimeVal b)

One param per line

@@ +82,3 @@
+timeval_compare (GTimeVal a, GTimeVal b)
+{
+  if ((a.tv_sec > b.tv_sec) || ((a.tv_sec == b.tv_sec) && (a.tv_usec > b.tv_usec)))

return ((a.tv_sec > b.tv_sec) || ((a.tv_sec == b.tv_sec) && (a.tv_usec > b.tv_usec)))

@@ +110,3 @@
+  all_files = g_ptr_array_new_with_free_func (g_object_unref);
+
+  for (int i = 0; i < changes->len ; i++)

guint i

@@ +116,3 @@
+      dir_files = ((IndexingData *)g_ptr_array_index (changes, i))->files;
+
+      for (int j = 0; j < dir_files->len; j++)

guint j

@@ +137,3 @@
+                                   GFile                *file,
+                                   guint32               file_id,
+                                   IdePersistentMapBuilder        *map_builder,

mind alignment

@@ +167,3 @@
+      IdeCodeIndexEntry *entry;
+
+      g_snprintf (num, 100, "%d", file_id);

don't duplicate sizes, they can get out of sync. sizeof num

@@ +195,3 @@
+                                          NULL,
+                                          NULL);
+

Add a check for 0-based begin_line/offset and set them to 1 if so (so that -1 later on stays valid). They shouldn't be, but we can consider them "unset" in this case I guess.

@@ +199,3 @@
+            ide_persistent_map_builder_insert (map_builder,
+                                    usr,
+                                    g_variant_new ("(uuuu)",

Alignment

@@ +264,3 @@
+
+  if (!ide_persistent_map_builder_write (map_builder,
+                              keys_file,

Alignment

@@ +343,3 @@
+  g_task_set_task_data (dirs_task, GUINT_TO_POINTER (changes->len), NULL);
+
+  for (int i = 0; i < changes->len; i++)

guint i

@@ +471,3 @@
+    }
+
+  for (int i = 0; i < directories->len; i++)

guint i

@@ +473,3 @@
+  for (int i = 0; i < directories->len; i++)
+    {
+      gchar *file_name;

const

@@ +638,3 @@
+
+  /* Update build_flags hash table with new flags */
+  g_hash_table_iter_init (&iter, self->build_flags);

This seems incorrect, you're both iterating self->build_flags and inserting back into self->build_flags. Should one of these just be "build_flags"?

::: plugins/code-index/ide-code-index-builder.h
@@ +42,3 @@
+                                                                GError                 **error);
+IdeCodeIndexBuilder    *ide_code_index_builder_new             (IdeContext              *context,
+	                                                            IdeCodeIndexIndex       *index,

Mind the alignment.
Comment 29 Christian Hergert 2017-08-26 02:01:47 UTC
Review of attachment 358276 [details] [review]:

Cool, just some questions to make things more readable in the future.

::: plugins/code-index/ide-code-index-index.c
@@ +25,3 @@
+#include "ide-persistent-map.h"
+
+#define MAX_LEN 20

Too generic of a define, we should be more specific in the naming so it's obvious what it does when reading the define name out of context.

@@ +86,3 @@
+  g_clear_pointer (&data->query, g_free);
+
+  for (int i = 0; i < data->fuzzy_matches->len; i++)

guint

@@ +299,3 @@
+static IdeCodeIndexSearchResult *
+ide_code_index_index_new_search_result (IdeCodeIndexIndex *self,
+                                        FuzzyMatch        *fuzzy_match)

FuzzyMatch can probably be const?

@@ +341,3 @@
+
+  return ide_code_index_search_result_new (context,
+                                           title + 2,

Maybe use a define for this? I'm assuming it's the "type prefix" like "c " or something like that?

@@ +576,3 @@
+    }
+
+  g_snprintf (num, MAX_LEN, "%d", file_id);

sizeof num instead of MAX_LEN

::: plugins/code-index/ide-code-index-index.h
@@ +29,3 @@
+
+IdeCodeIndexIndex *ide_code_index_index_new               (IdeContext            *context);
+/* This function will load index from directory if it is not modified. This fuction will only

Comments like this belong in the .c file with the gtk-doc for the function.
Comment 30 Christian Hergert 2017-08-26 02:14:31 UTC
Review of attachment 358278 [details] [review]:

::: plugins/clang/ide-clang-code-indexer.c
@@ +123,3 @@
+
+      /* entries has to dispose TU when done with it */
+      entries = ide_clang_code_index_entries_new (&tu, filename);

Why is a stack address being passed? If it steals ownership, why not just g_steal_pointer (&tu)?

@@ +149,3 @@
+  g_autofree gchar *key;
+  IdeSourceLocation *location;
+

assertion precondition checks
Comment 31 Christian Hergert 2017-08-26 02:46:09 UTC
Review of attachment 358281 [details] [review]:

::: plugins/code-index/ide-code-index-search-provider.c
@@ +63,3 @@
+  IdeCodeIndexService *service;
+  IdeCodeIndexIndex *index;
+  GTask *task;

g_autoptr(GTask) task = NULL;
Comment 32 Christian Hergert 2017-08-26 02:48:57 UTC
Review of attachment 358280 [details] [review]:

Use boxed for the :location property, then looks good.

::: plugins/code-index/ide-code-index-search-result.c
@@ +65,3 @@
+
+    case PROP_LOCATION:
+      g_value_set_pointer (value, self->location);

set_boxed

@@ +88,3 @@
+
+    case PROP_LOCATION:
+      self->location = ide_source_location_ref (g_value_get_pointer (value));

self->location = g_value_dup_boxed (value);

@@ +127,3 @@
+
+  properties [PROP_LOCATION] =
+    g_param_spec_pointer ("location",

This should be a g_param_spec_boxed() with IDE_TYPE_SOURCE_LOCATION
Comment 33 Christian Hergert 2017-08-26 02:50:50 UTC
Review of attachment 358286 [details] [review]:

::: libide/buildsystem/ide-build-system.c
@@ +116,3 @@
                          g_steal_pointer (&flags));
 
+  if (++data->index < data->files->len)

I don't like increment operators and comparison operators mixed. Keep these separate.
Comment 34 Christian Hergert 2017-08-26 02:52:31 UTC
Review of attachment 358287 [details] [review]:

::: libide/ide-context.c
@@ +2078,3 @@
                         NULL);
+
+  g_clear_object (&self->device_manager);

This looks brittle to me. We should wait to clear the manager objects as a final async callback step perhaps?
Comment 35 Christian Hergert 2017-08-26 02:53:18 UTC
Review of attachment 358288 [details] [review]:

Cool
Comment 36 Christian Hergert 2017-08-26 02:54:10 UTC
Review of attachment 358285 [details] [review]:

Cool
Comment 37 Christian Hergert 2017-08-26 02:55:29 UTC
Review of attachment 358284 [details] [review]:

Cool
Comment 38 Christian Hergert 2017-08-26 03:05:52 UTC
Review of attachment 358282 [details] [review]:

Cool
Comment 39 Christian Hergert 2017-08-26 03:22:46 UTC
Review of attachment 358279 [details] [review]:

I'm concerned that GListModel isn't helping here. Can we stop using GListModel?

::: plugins/clang/ide-clang-code-index-entries.c
@@ +42,3 @@
+  PROP_0,
+  PROP_MAIN_FILE,
+  PROP_TU,

This is an unhelpful property name. Maybe "unit" if you think "translation-unit" is too long.

@@ +56,3 @@
+static void
+cx_cursor_free (CXCursor *cursor,
+                gpointer  user_data)

drop user_data from parameters

@@ +66,3 @@
+ */
+static enum CXChildVisitResult
+visitor (CXCursor cursor, CXCursor parent, CXClientData client_data)

One parameter per line

@@ +68,3 @@
+visitor (CXCursor cursor, CXCursor parent, CXClientData client_data)
+{
+  IdeClangCodeIndexEntries *self = (IdeClangCodeIndexEntries *)client_data;

No cast is necessary -> typedef void *CXClientData;

@@ +217,3 @@
+      break;
+    case CXCursor_VarDecl:
+      kind = IDE_SYMBOL_VARIABLE;

break is missing. Remove this kind =  and just fallthrough to the following case.

@@ +230,3 @@
+      kind = IDE_SYMBOL_NAMESPACE;
+      break;
+    case CXCursor_FunctionTemplate:

Share case blocks for items that use the same kind.

case CXCursor_FunctionTemplate:
case CXCursor_ClassTemplate:
  kind = IDE_SYMBOL_TEMPLATE;
  break;

@@ +252,3 @@
+  /* Add prefix to name so that filters can be applied */
+  if (kind == IDE_SYMBOL_FUNCTION)
+    prefix = "f\x1F";

We should probably add a new define in ide-macros.h for special string separators.

@@ +318,3 @@
+
+  while (data == NULL && !finish)
+    data = ide_clang_code_index_entries_real_get_item (model, position, &finish);

This seems like a strange way to do this. It seems like it's shoe-horning things into using GListModel. If GListModel isn't helping, we shouldn't implement it.

@@ +451,3 @@
+
+IdeClangCodeIndexEntries*
+ide_clang_code_index_entries_new (CXTranslationUnit *tu,

Isn't CXTranslationUnit already a pointer? Why add *?

::: plugins/clang/ide-clang-code-index-entries.h
@@ +29,3 @@
+G_DECLARE_FINAL_TYPE (IdeClangCodeIndexEntries, ide_clang_code_index_entries, IDE, CLANG_CODE_INDEX_ENTRIES, GObject)
+
+IdeClangCodeIndexEntries *ide_clang_code_index_entries_new (CXTranslationUnit *tu,

Let's drop the * on tu, it's already a pointer.

(from Index.h)
typedef struct CXTranslationUnitImpl *CXTranslationUnit;

@@ +30,3 @@
+
+IdeClangCodeIndexEntries *ide_clang_code_index_entries_new (CXTranslationUnit *tu,
+                                                            gchar             *source_filename);

const gchar *source_filename

::: plugins/clang/ide-clang-code-index-entry.c
@@ +63,3 @@
+  g_return_val_if_fail (IDE_IS_CLANG_CODE_INDEX_ENTRY (self), NULL);
+
+  return self->usr;

return g_strdup (self->usr) (as we want interfaces to return string copies so they work with PyGObject correctly)

::: plugins/clang/ide-clang-code-index-entry.h
@@ +38,3 @@
+                                                        guint                   end_line_offset);
+
+IdeClangCodeIndexEntry *ide_clang_code_index_entry_new ();

(void)
Comment 40 Christian Hergert 2017-08-26 03:42:15 UTC
Review of attachment 358283 [details] [review]:

Couple correctness things to fix, then this looks good. Nice work.

::: libide/plugins/ide-extension-set-adapter.c
@@ +210,3 @@
         }
     }
+    g_signal_emit (self, signals [EXTENSIONS_LOADED], 0, self);

miss-alignment and trailing "self" should not be necessary.

@@ +455,3 @@
+                  G_TYPE_NONE,
+                  1,
+                  G_SIGNAL_RUN_LAST,

This isn't right, self is already passed as the first argument, so this should be:

 G_TYPE_NONE, 0);

::: libide/sourceview/ide-source-view.c
@@ +6009,3 @@
 {
+  IdeSymbolResolver *symbol_resolver = (IdeSymbolResolver *)object;
+  g_autoptr(IdeSourceView) self;

self should not be an autoptr, g_task_get_source_object() does not return a new reference.

::: tests/meson.build
@@ +17,3 @@
   'test-ide-context.c',
   c_args: ide_test_cflags,
+  dependencies: [

Create ide_test_deps and then use that like we do for cflags.
Comment 41 Anoop Chandu 2017-08-26 15:58:38 UTC
Created attachment 358473 [details] [review]
code-index: Adding IdeCodeIndexService to code-index

fixed
Comment 42 Anoop Chandu 2017-08-26 15:59:39 UTC
Created attachment 358474 [details] [review]
symbols: Adding IdeCodeIndexer,IdeCodeIndexEntry  interfaces

fixed
Comment 43 Anoop Chandu 2017-08-26 16:00:19 UTC
Created attachment 358475 [details] [review]
Adding persistent map

fixed
Comment 44 Anoop Chandu 2017-08-26 16:01:03 UTC
Created attachment 358476 [details] [review]
code-index: Adding IdeCodeIndexBuilder

fixed
Comment 45 Anoop Chandu 2017-08-26 16:02:43 UTC
Created attachment 358477 [details] [review]
code-index: Adding IdeCodeIndexIndex

fixed
Comment 46 Anoop Chandu 2017-08-26 16:03:17 UTC
Created attachment 358478 [details] [review]
clang: Implement IdeCodeIndexer

fixed
Comment 47 Anoop Chandu 2017-08-26 16:04:02 UTC
Created attachment 358479 [details] [review]
code-index: Implementing IdeSearchResult

fixed
Comment 48 Anoop Chandu 2017-08-26 16:04:57 UTC
Created attachment 358480 [details] [review]
code-index: Implmenting search provider

fixed
Comment 49 Anoop Chandu 2017-08-26 16:05:24 UTC
Created attachment 358481 [details] [review]
code-index: Implementing symbol resolver

fixed
Comment 50 Anoop Chandu 2017-08-26 16:06:04 UTC
Created attachment 358482 [details] [review]
buffer: Use multiple symbol resolvers instead of one.

fixed
Comment 51 Anoop Chandu 2017-08-26 16:07:28 UTC
Created attachment 358483 [details] [review]
buildsystem: Handle empty file array and task cancellation

fixed
Comment 52 Anoop Chandu 2017-08-26 16:09:18 UTC
Created attachment 358484 [details] [review]
libide: Unload services before destroying  RuntimeManager

fixed
Comment 53 Anoop Chandu 2017-08-26 16:10:48 UTC
Created attachment 358485 [details] [review]
symbols: Adding IdeCodeIndexEntries interface

New interface added.
Comment 54 Anoop Chandu 2017-08-26 16:12:17 UTC
Created attachment 358486 [details] [review]
clang: Implement IdeCodeIndexEntries and IdeCodeIndexEntry
Comment 55 Anoop Chandu 2017-08-27 06:02:49 UTC
Created attachment 358500 [details] [review]
code-index: fixes in IdeCodeIndexBuilder and IdePersistentMap

few bugs are fixed. When IdeCodeIndexer is null in IdeCodeIndexBuilder dont proceed to index file. When target value is found in IdePersistentMap then stop searching.
Comment 56 Christian Hergert 2017-08-28 07:02:03 UTC
Review of attachment 358473 [details] [review]:

Fix the crasher and then this should be good to go

::: plugins/code-index/ide-code-index-service.c
@@ +123,3 @@
+                                          bdata->directory,
+                                          bdata->recursive,
+                                          self->cancellable,

This is going to crash. You need to make the GCancellable a local because self will be made NULL before this dereference occurs.
Comment 57 Christian Hergert 2017-08-28 07:08:55 UTC
Review of attachment 358474 [details] [review]:

LGTM
Comment 58 Christian Hergert 2017-08-28 07:17:56 UTC
Review of attachment 358475 [details] [review]:

LGTM
Comment 59 Christian Hergert 2017-08-28 07:43:15 UTC
Review of attachment 358476 [details] [review]:

Couple nits, otherwise LGTM

::: plugins/code-index/ide-code-index-builder.c
@@ +165,3 @@
+      IdeCodeIndexEntry *entry;
+
+      g_snprintf (num, 100, "%u", file_id);

100 -> sizeof num

@@ +207,3 @@
+                                            name,
+                                            g_variant_new ("(uuuuu)",
+                                            file_id,

Indent the g_variant_new() parameters so it is clear they are part of it

::: plugins/code-index/ide-code-index-builder.h
@@ +42,3 @@
+                                                                GError                 **error);
+IdeCodeIndexBuilder    *ide_code_index_builder_new             (IdeContext              *context,
+	                                                            IdeCodeIndexIndex       *index,

Alignment
Comment 60 Christian Hergert 2017-08-28 07:56:50 UTC
Review of attachment 358477 [details] [review]:

LGTM
Comment 61 Christian Hergert 2017-08-28 08:01:39 UTC
Review of attachment 358478 [details] [review]:

LGTM
Comment 62 Christian Hergert 2017-08-28 08:03:33 UTC
Review of attachment 358479 [details] [review]:

LGTM
Comment 63 Christian Hergert 2017-08-28 08:06:40 UTC
Review of attachment 358480 [details] [review]:

LGTM
Comment 64 Christian Hergert 2017-08-28 08:10:17 UTC
Review of attachment 358481 [details] [review]:

LGTM
Comment 65 Christian Hergert 2017-08-28 08:27:59 UTC
Review of attachment 358482 [details] [review]:

LGTM

::: tests/meson.build
@@ +15,3 @@
 
+ide_test_deps = [
+  libpeas_dep,

add libide_dep here and then remove it from the entries below
Comment 66 Christian Hergert 2017-08-28 08:29:50 UTC
Review of attachment 358483 [details] [review]:

LGTM
Comment 67 Christian Hergert 2017-08-28 08:32:15 UTC
Review of attachment 358484 [details] [review]:

Hrmm, I was thinking of adding a new unload async cb for unref'ing these, but this seems fine too.
Comment 68 Christian Hergert 2017-08-28 08:35:15 UTC
Review of attachment 358485 [details] [review]:

Adjust things to allow a full reference return (possibly in consuming patches too) and then this is good.

::: libide/symbols/ide-code-index-entries.c
@@ +41,3 @@
+ * This will fetch next entry in index.
+ *
+ * Returns: (transfer none) : An #IdeCodeIndexEntry.

We need a (transfer full) here so that we can do lazy expansion of items by creating each Entry object on demand.
Comment 69 Christian Hergert 2017-08-28 08:46:58 UTC
Review of attachment 358486 [details] [review]:

Couple quick things, and adjust for the (transfer full)

::: plugins/clang/ide-clang-code-index-entries.c
@@ +308,3 @@
+    entry = ide_clang_code_index_entries_real_get_next_entry (self, &finish);
+
+  return (IdeCodeIndexEntry *)entry;

Return a g_object_ref(self->entry) here so that we allow ourselves some flexibility in the future to create new objects on demand.

@@ +339,3 @@
+  G_OBJECT_CLASS (ide_clang_code_index_entries_parent_class)->constructed (object);
+
+  root_cursor = g_slice_new (CXCursor);

You can also do something like:

CXCursor cursor;

cursor = clang_getTranslationUnitCursor (self->tu);
g_queue_push_head (&self->cursors, g_slice_dup (CXCursor, &cursor));

@@ +399,3 @@
+
+  properties [PROP_MAIN_FILE] =
+    g_param_spec_pointer ("main_file",

- instead of _ for property names

@@ +433,3 @@
+  return g_object_new (IDE_TYPE_CLANG_CODE_INDEX_ENTRIES,
+                       "unit", unit,
+                       "main_file", main_file,

I know that - and _ both work for property names, but use - to be consistent.
Comment 70 Christian Hergert 2017-08-28 08:49:09 UTC
Review of attachment 358500 [details] [review]:

LGTM
Comment 71 Anoop Chandu 2017-08-29 16:02:44 UTC
Created attachment 358695 [details] [review]
symbols: Adding IdeCodeIndexEntries interface

complete transfer of IdeCodeIndexEntry.
Comment 72 Anoop Chandu 2017-08-29 16:04:53 UTC
Created attachment 358696 [details] [review]
symbols: Adding IdeCodeIndexer & IdeCodeIndexEntry

IdeCodeIndexEntry is now a class instead of interface. Implementation of IdeCodeIndexEntries will return an instance of IdeCodeIndexEntry by setting properties in it.
Comment 73 Anoop Chandu 2017-08-29 16:06:52 UTC
Created attachment 358697 [details] [review]
clang: Implement IdeCodeIndexEntries

IdeClangCodeIndexEntries now return an instance of IdeCodeIndexEntry (transfer full).
Comment 74 Anoop Chandu 2017-08-29 16:07:59 UTC
Created attachment 358698 [details] [review]
code-index: Adding IdeCodeIndexBuilder

fixed.
Comment 75 Anoop Chandu 2017-08-29 16:09:17 UTC
Created attachment 358699 [details] [review]
code-index: Adding IdeCodeIndexService to code-index

error using g_steal_pointer is fixed.
Comment 76 Anoop Chandu 2017-08-29 16:10:59 UTC
Created attachment 358700 [details] [review]
clang: Adding code indexer

IdeClangCodeIndexEntry class files removed.
Comment 77 Anoop Chandu 2017-08-29 16:13:11 UTC
Created attachment 358705 [details] [review]
buffer: Use multiple symbol resolvers instead of one.

libide_dep added to ide_test_deps in tests/meson.build
Comment 78 Anoop Chandu 2017-08-30 08:08:01 UTC
Comment on attachment 358500 [details] [review]
code-index: fixes in IdeCodeIndexBuilder and IdePersistentMap

Changes are included in this https://bugzilla.gnome.org/show_bug.cgi?id=786700#c74 patch.
Comment 79 Christian Hergert 2017-09-01 22:05:39 UTC
I'm getting an infinite loop somewhere due to this. I think it starts from

  ide_code_index_symbol_resolver_lookup_cb()

  • #0 g_bit_unlock
    at /home/christian/jhbuild/checkout/glib/glib/gbitlock.c line 325
  • #1 g_variant_unlock
    at /home/christian/jhbuild/checkout/glib/glib/gvariant-core.c line 233
  • #2 g_variant_n_children
    at /home/christian/jhbuild/checkout/glib/glib/gvariant-core.c line 944
  • #3 g_variant_get_child_value
    at /home/christian/jhbuild/checkout/glib/glib/gvariant-core.c line 975
  • #4 ide_persistent_map_lookup_value
    at ../plugins/code-index/ide-persistent-map.c line 277
  • #5 ide_code_index_index_lookup_symbol
    at ../plugins/code-index/ide-code-index-index.c line 562
  • #6 ide_code_index_symbol_resolver_lookup_cb
    at ../plugins/code-index/ide-code-index-symbol-resolver.c line 67
  • #7 g_task_return_now
    at /home/christian/jhbuild/checkout/glib/gio/gtask.c line 1145
  • #8 g_task_return
    at /home/christian/jhbuild/checkout/glib/gio/gtask.c line 1203
  • #9 g_task_return_pointer
    at /home/christian/jhbuild/checkout/glib/gio/gtask.c line 1601
  • #10 ide_clang_code_indexer_generate_key_cb
    at ../plugins/clang/ide-clang-code-indexer.c line 171
  • #11 g_task_return_now
    at /home/christian/jhbuild/checkout/glib/gio/gtask.c line 1145
  • #12 complete_in_idle_cb
    at /home/christian/jhbuild/checkout/glib/gio/gtask.c line 1159
  • #13 g_idle_dispatch
    at /home/christian/jhbuild/checkout/glib/glib/gmain.c line 5504
  • #14 g_main_dispatch
    at /home/christian/jhbuild/checkout/glib/glib/gmain.c line 3148
  • #15 g_main_context_dispatch
    at /home/christian/jhbuild/checkout/glib/glib/gmain.c line 3813
  • #16 g_main_context_iterate
    at /home/christian/jhbuild/checkout/glib/glib/gmain.c line 3886
  • #17 g_main_context_iteration
    at /home/christian/jhbuild/checkout/glib/glib/gmain.c line 3947
  • #18 g_application_run
    at /home/christian/jhbuild/checkout/glib/gio/gapplication.c line 2401
  • #19 main
    at ../src/main.c line 123

Comment 80 Christian Hergert 2017-09-01 22:23:02 UTC
Created attachment 358962 [details] [review]
code-index: fix infinte loop in binary search
Comment 81 Christian Hergert 2017-09-01 22:24:52 UTC
Excellent work, I'm really happy to see this merged.

I think it's in good enough shape that if/when we find issues, we can just keep
plugging away at things on the master branch.

Attachment 358272 [details] pushed as 67f3085 - symbols: Added symbols, flags and icons
Attachment 358284 [details] pushed as 35f35a6 - code-index: Adding plugin files
Attachment 358475 [details] pushed as 103df38 - Adding persistent map
Attachment 358477 [details] pushed as 73c8e7e - code-index: Adding IdeCodeIndexIndex
Attachment 358478 [details] pushed as bd27c6e - clang: Implement IdeCodeIndexer
Attachment 358479 [details] pushed as 8a38038 - code-index: Implementing IdeSearchResult
Attachment 358480 [details] pushed as ac3eb3e - code-index: Implmenting search provider
Attachment 358481 [details] pushed as 8056ad5 - code-index: Implementing symbol resolver
Attachment 358483 [details] pushed as 0cc6d80 - buildsystem: Handle empty file array and task cancellation
Attachment 358695 [details] pushed as 80118f9 - symbols: Adding IdeCodeIndexEntries interface
Attachment 358696 [details] pushed as a859535 - symbols: Adding IdeCodeIndexer & IdeCodeIndexEntry
Attachment 358697 [details] pushed as 816d550 - clang: Implement IdeCodeIndexEntries
Attachment 358698 [details] pushed as 4e77909 - code-index: Adding IdeCodeIndexBuilder
Attachment 358699 [details] pushed as aec820a - code-index: Adding IdeCodeIndexService to code-index
Attachment 358700 [details] pushed as 0ac0135 - clang: Adding code indexer
Attachment 358705 [details] pushed as 6f17acd - buffer: Use multiple symbol resolvers instead of one.
Attachment 358962 [details] pushed as fb924f1 - code-index: fix infinte loop in binary search
Comment 82 Anoop Chandu 2017-09-02 11:31:40 UTC
Created attachment 358975 [details] [review]
code-index: Avoid infinite build loop

Added a variable which keeps track of number of trials are made for bulding index. If current trial number exceeds maximum then next trial for building wont be made.
Comment 83 Anoop Chandu 2017-09-02 13:00:41 UTC
Created attachment 358981 [details] [review]
code-index: Avoid segfault when indexing is stopped
Comment 84 Christian Hergert 2017-09-02 18:54:44 UTC
Review of attachment 358975 [details] [review]:

LGTM
Comment 85 Christian Hergert 2017-09-02 18:55:13 UTC
Review of attachment 358981 [details] [review]:

LGTM
Comment 86 Christian Hergert 2017-09-02 19:13:42 UTC
Comment on attachment 358975 [details] [review]
code-index: Avoid infinite build loop

Attachment 358975 [details] pushed as 240bca3 - code-index: Avoid infinite build loop