Skip to content
This repository has been archived by the owner on Mar 8, 2020. It is now read-only.

encode() and a stub for the load() #89

Merged
merged 5 commits into from
Jun 14, 2019
Merged

Conversation

bzz
Copy link
Contributor

@bzz bzz commented Jun 6, 2019

Another part of #83

  • Context.encode() implementation + tests
  • a stub for Node.load()

Actual load() implementation will be done in subsequent PRs.

bzz added 2 commits June 6, 2019 19:18
Signed-off-by: Alexander Bezzubov <bzz@apache.org>
Signed-off-by: Alexander Bezzubov <bzz@apache.org>
@bzz bzz self-assigned this Jun 6, 2019
Signed-off-by: Alexander Bezzubov <bzz@apache.org>
Copy link
Contributor

@creachadair creachadair left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just a couple documentation-related requests.

bzz added 2 commits June 11, 2019 11:50
Signed-off-by: Alexander Bezzubov <bzz@apache.org>
Signed-off-by: Alexander Bezzubov <bzz@apache.org>
@bzz
Copy link
Contributor Author

bzz commented Jun 11, 2019

Thank you for prompt reviews!
@dennwc all feedback addressed, ready for another round.

@@ -15,6 +15,8 @@ jobject asJvmBuffer(uast::Buffer buf) {
return env->NewDirectByteBuffer(buf.ptr, buf.size);
}

const char *nativeContext = "nativeContext";
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Minor nit: Either const char * const nativeContext = "…" or const char nativeContext[] = "…" is preferable here—to ensure the pointer itself is also const (either works). constexpr char nativeContext[] = "…" will also work if we assume C++ ≥ 11.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, thank you, we do! Was thinking about constexpr as well - will replace as part of the next PR.

@bzz bzz merged commit 2bc2167 into bblfsh:master Jun 14, 2019
@bzz bzz deleted the add-encode-load branch June 14, 2019 20:32
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants