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

Stream: git-cranelift

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

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

[x] This has been discussed in issue #..., or if not, please tell us why
here.

[x] A short description of what this does, why it is needed; if the
description becomes long, the matter should probably be discussed in an issue
first.

[ ] This PR contains test cases, if meaningful.

[x] A reviewer from the core maintainer team has been assigned for this PR.
If you don't know who could review this, please indicate so and/or ping
bnjbvr. The list of suggested reviewers on the right can help you.

GitHub (Jan 24 2020 at 08:17):

bnjbvr requested iximeow for a review on PR #1364.

GitHub (Jan 24 2020 at 18:00):

iximeow submitted PR Review.

GitHub (Jan 24 2020 at 18:00):

iximeow submitted PR Review.

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.

GitHub (Jan 24 2020 at 21:20):

philipc submitted PR Review.

GitHub (Jan 24 2020 at 21:20):

philipc created PR Review Comment:

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

GitHub (Jan 24 2020 at 21:26):

iximeow submitted PR Review.

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.

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

[x] This has been discussed in issue #..., or if not, please tell us why
here.

[x] A short description of what this does, why it is needed; if the
description becomes long, the matter should probably be discussed in an issue
first.

[ ] This PR contains test cases, if meaningful.

[x] A reviewer from the core maintainer team has been assigned for this PR.
If you don't know who could review this, please indicate so and/or ping
bnjbvr. The list of suggested reviewers on the right can help you.

GitHub (Jan 25 2020 at 03:22):

philipc submitted PR Review.

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.

GitHub (Jan 25 2020 at 19:28):

iximeow submitted PR Review.

GitHub (Jan 25 2020 at 19:28):

iximeow created PR Review Comment:

Yeah I think that's a good call.

GitHub (Jan 25 2020 at 19:28):

iximeow submitted PR Review.

GitHub (Feb 10 2020 at 10:42):

bnjbvr merged PR #1364.

Last updated: Nov 22 2024 at 16:03 UTC