Filter implementation

[bzz]

Addresses the last major part of the #83 - XPath query/filter implementation

TODOs

• XPath query for native nodes
• XPath query for managed nodes
• high-level API in client for both cases
• make all tests pass
• addressing the last FIXME

---

This change is 

[bzz]

Overall, all moving parts are there and it's ready for the feedback.

CI is expected to fail; will fix the last todo item, a failing test, first thing in the morning.

[bzz]

@creachadair CI is green and it's ready for a review

[creachadair]

I just have a couple of small comments; otherwise I think this looks good.

Reviewed 9 of 17 files at r1, 8 of 8 files at r3, 2 of 3 files at r4.
Reviewable status: 16 of 17 files reviewed, 4 unresolved discussions (waiting on @bzz, @creachadair, @dennwc, and @ncordon)

---

src/main/native/org_bblfsh_client_v2_libuast_Libuast.cc, line 157 at r3 (raw file):

  // Filter queries an external UAST.
  // Borrows the reference.

Does this mean that while the filter is active, it is not safe to use the node? (That's ok if so: But we should probably document it, since I think it may mean we must fully exhaust or explicitly release the iterator to ensure the node returns to a valid state in case of error)

---

src/main/scala/org/bblfsh/client/v2/libuast/Libuast.scala, line 17 at r4 (raw file):

  if (!loaded) {
    println("Loading native libscalauast")

Is this meant to be to stderr and/or a log? (Otherwise anyone who imports this library will spam their stdout)

---

src/test/scala/org/bblfsh/client/v2/FilterManagedTest.scala, line 22 at r4 (raw file):

  override def beforeAll() = {
    println(s"Libuast.loaded: ${Libuast.loaded}")

Was this mean to be a log and/or to stderr?

---

src/test/scala/org/bblfsh/client/v2/FilterNativeTest.scala, line 42 at r3 (raw file):

    val pos = it.toList
    pos should have size (8)

Please add a comment to say a bit about what this means—why 8 instead of 37? (I think the answer is "Tiny.java contains 8 tokens" or something like that)

[bzz]

Reviewable status: 13 of 17 files reviewed, all discussions resolved (waiting on @creachadair, @dennwc, and @ncordon)

---

src/main/native/org_bblfsh_client_v2_libuast_Libuast.cc, line 157 at r3 (raw file):

Does this mean that while the filter is active, it is not safe to use the node?

No, not really - lifetime of the node is managed by the context and the same node can be used by multiple filters.

We use this language consistently to signify that the references to the managed JVM Node object is not retained on the native side, and thus no extra JNI cleanup is needed afterwards.

---

src/main/scala/org/bblfsh/client/v2/libuast/Libuast.scala, line 17 at r4 (raw file):

Previously, creachadair (M. J. Fromberger) wrote…

Is this meant to be to stderr and/or a log? (Otherwise anyone who imports this library will spam their stdout)

Yes it is. Changed to stderr that should play nice with the logging.

---

src/test/scala/org/bblfsh/client/v2/FilterManagedTest.scala, line 22 at r4 (raw file):

Previously, creachadair (M. J. Fromberger) wrote…

Was this mean to be a log and/or to stderr?

Yes, updated.

---

src/test/scala/org/bblfsh/client/v2/FilterNativeTest.scala, line 42 at r3 (raw file):

Previously, creachadair (M. J. Fromberger) wrote…

Please add a comment to say a bit about what this means—why 8 instead of 37? (I think the answer is "Tiny.java contains 8 tokens" or something like that)

Done

[bzz]

All feedback has been addressed, it's ready for another round.

[creachadair]

Reviewed 5 of 5 files at r5.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @dennwc and @ncordon)

---

src/main/native/org_bblfsh_client_v2_libuast_Libuast.cc, line 157 at r3 (raw file):

Previously, bzz (Alexander) wrote…

Does this mean that while the filter is active, it is not safe to use the node?

No, not really - lifetime of the node is managed by the context and the same node can be used by multiple filters.

We use this language consistently to signify that the references to the managed JVM Node object is not retained on the native side, and thus no extra JNI cleanup is needed afterwards.

👍

[ncordon]

Looked good to me, sorry, I arrived late, but I did looked at the code. I was filing an issue non-related to this PR in the repo :)