Stream: git-cranelift

Topic: cranelift / PR #1364 cranelift-object: move relocation pr...


view this post on Zulip GitHub (Jan 24 2020 at 04:21):

philipc opened PR #1364 from issue-1353 to master:

This removes the need to call finalize_definitions for cranelift-object. finalize_definitions is only intended for backends that produce finalized functions and data objects, which cranelift-object does not.

Fixes #1353

I couldn't come up with better names for finalize_definitions and finish. I think improving the documentation is enough. I made a small improvement here for finalize_definitions. I think it was already pretty clear what finish did.

r? @iximeow

<!-- Please ensure all communication adheres to the code of conduct. -->

view this post on Zulip GitHub (Jan 24 2020 at 08:17):

bnjbvr requested iximeow for a review on PR #1364.

view this post on Zulip GitHub (Jan 24 2020 at 18:00):

iximeow submitted PR Review.

view this post on Zulip GitHub (Jan 24 2020 at 18:00):

iximeow submitted PR Review.

view this post on Zulip GitHub (Jan 24 2020 at 18:00):

iximeow created PR Review Comment:

I think a point of confusion has been what "finalized" means in contrast to whatever "finish" might do. Not having better names for these either, can we clarify what the difference is? Just noting that some backends have a meaningful step in their lifecycle for "functions and data that can be used, not necessarily written to an output" would have clarified a lot for me, at least.

view this post on Zulip GitHub (Jan 24 2020 at 21:20):

philipc submitted PR Review.

view this post on Zulip GitHub (Jan 24 2020 at 21:20):

philipc created PR Review Comment:

What if I say it's only relevant for JIT backends?

view this post on Zulip GitHub (Jan 24 2020 at 21:26):

iximeow submitted PR Review.

view this post on Zulip GitHub (Jan 24 2020 at 21:26):

iximeow created PR Review Comment:

That's a bit more prescriptive than I prefer for documentation, but I won't push back on it for lack of counterexample.

view this post on Zulip GitHub (Jan 25 2020 at 03:20):

philipc updated PR #1364 from issue-1353 to master:

This removes the need to call finalize_definitions for cranelift-object. finalize_definitions is only intended for backends that produce finalized functions and data objects, which cranelift-object does not.

Fixes #1353

I couldn't come up with better names for finalize_definitions and finish. I think improving the documentation is enough. I made a small improvement here for finalize_definitions. I think it was already pretty clear what finish did.

r? @iximeow

<!-- Please ensure all communication adheres to the code of conduct. -->

view this post on Zulip GitHub (Jan 25 2020 at 03:22):

philipc submitted PR Review.

view this post on Zulip GitHub (Jan 25 2020 at 03:22):

philipc created PR Review Comment:

Ok, so instead of using ambiguous English, let's refer to the condition in the code instead. I've moved this change to a new commit that also updates the comments on other functions.

view this post on Zulip GitHub (Jan 25 2020 at 19:28):

iximeow submitted PR Review.

view this post on Zulip GitHub (Jan 25 2020 at 19:28):

iximeow created PR Review Comment:

Yeah I think that's a good call.

view this post on Zulip GitHub (Jan 25 2020 at 19:28):

iximeow submitted PR Review.

view this post on Zulip GitHub (Feb 10 2020 at 10:42):

bnjbvr merged PR #1364.


Last updated: Dec 23 2024 at 14:03 UTC