-
Notifications
You must be signed in to change notification settings - Fork 8
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed 5 of 5 files at r1.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @bzz and @dennwc)
README.md, line 9 at r1 (raw file):
### Status Latest`scala-client` v2.x supports [UASTv2 protocol](https://doc.bblf.sh/uast/uast-specification-v2.html).
Minor tweaks:
The latest scala-client
v2.x supports the UASTv2 protocol
viz., missing space after "latest", adds articles.
src/main/native/org_bblfsh_client_v2_libuast_Libuast.cc, line 404 at r1 (raw file):
}; struct EqByObj {
Please add brief comments to say what these types are for. (I went and looked at the usage, but that won't be obvious later without the diff 🙂)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: 3 of 6 files reviewed, 1 unresolved discussion (waiting on @creachadair and @dennwc)
README.md, line 9 at r1 (raw file):
Previously, creachadair (M. J. Fromberger) wrote…
Minor tweaks:
The latest
scala-client
v2.x supports the UASTv2 protocolviz., missing space after "latest", adds articles.
Done.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed 3 of 3 files at r2.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @creachadair)
Investigating the CI test failure. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed 3 of 3 files at r3.
Reviewable status: all files reviewed, 7 unresolved discussions (waiting on @bzz and @creachadair)
CONTRIBUTING.md, line 41 at r3 (raw file):
./sbt "testOnly org.bblfsh.client.v2.libuast.IteratorNativeTest -- -z "Native UAST iterator should return non-empty results on decoded objects""
(optionally) consider using 'single quotes'
around the outside argument, to avoid the need for nested escapes.
CONTRIBUTING.md, line 44 at r3 (raw file):
Simpler:
When using
lldb
, the classpath needs to be manually set for thejava
executable:
CONTRIBUTING.md, line 58 at r3 (raw file):
Suggestion:
These instructions are for
lldb
, but the steps should be similar ingdb
:
CONTRIBUTING.md, line 68 at r3 (raw file):
If that does not load the symbols, you have to make sure libscalauast library has
I suggest code-quoting libscalauast
.
CONTRIBUTING.md, line 70 at r3 (raw file):
If that does not load the symbols, you have to make sure libscalauast library has already been loaded to the process by `target modules list`. On rare occasions, the library can be loaded but the symbols will still refuse
I'm not sure I follow why this is rare: If the library is at the wrong path, won't it always fail?
Or perhaps you meant something like "If the library loads but symbols are not correctly displayed, it probably means the library is at the wrong filesystem path. Stop…" or words to that effect?
CONTRIBUTING.md, line 99 at r3 (raw file):
## JNI details Here are some resources to understand the JNI machinery and it's best practices:
nit: s/it's/its/
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: 9 of 20 files reviewed, 3 unresolved discussions (waiting on @bzz and @creachadair)
CONTRIBUTING.md, line 44 at r3 (raw file):
Previously, creachadair (M. J. Fromberger) wrote…
Simpler:
When using
lldb
, the classpath needs to be manually set for thejava
executable:
Done.
CONTRIBUTING.md, line 58 at r3 (raw file):
Previously, creachadair (M. J. Fromberger) wrote…
Suggestion:
These instructions are for
lldb
, but the steps should be similar ingdb
:
Done.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed 12 of 12 files at r4.
Reviewable status:complete! all files reviewed, all discussions resolved
Signed-off-by: Alexander Bezzubov <[email protected]>
Signed-off-by: Alexander Bezzubov <[email protected]>
Signed-off-by: Alexander Bezzubov <[email protected]>
Signed-off-by: Alexander Bezzubov <[email protected]>
Signed-off-by: Alexander Bezzubov <[email protected]>
Signed-off-by: Alexander Bezzubov <[email protected]>
Signed-off-by: Alexander Bezzubov <[email protected]>
Signed-off-by: Alexander Bezzubov <[email protected]>
Part of the memory leak work (that failed CI for linux) was fractioned out to separate #116 and will be addressed later. |
Addresses the last part of the #83 needed for the release:
TODOs:
address part of the Node::ValueAt may be leaking memory #113 by changing native map cache implementationmoved to separate Avoid using JNI refs as map keys #116
This change is