This is an archive of the discontinued LLVM Phabricator instance.

Reformulate OrcJIT tutorial doc to make it more clear.
ClosedPublic

Authored by sonson on Jul 18 2021, 5:35 AM.

Details

Summary

Fixed a minor writing error. The text was hard to understand.

Diff Detail

Event Timeline

sonson requested review of this revision.Jul 18 2021, 5:35 AM
sonson created this revision.
Herald added a project: Restricted Project. · View Herald TranscriptJul 18 2021, 5:35 AM
mehdi_amini added inline comments.Jul 19 2021, 9:39 AM
llvm/docs/tutorial/BuildingAJIT2.rst
229–230

Probably should reflow here to preserve the 80cols

230

Nit: there is a trailing space here.

230

I think these two added new lines aren't needed, are they?

231

"Each layer can complete its own work ..." ?

234

Nit: You have some initial empty space at the beginning of these lines apparently

sonson updated this revision to Diff 360357.EditedJul 20 2021, 10:07 PM

I fixed it as instructed by the reviewer.
Removed trailing spaces and no needed empty spaces.
Preserve 80cols at all lines.

Thank you.

mehdi_amini added inline comments.Jul 20 2021, 11:00 PM
llvm/docs/tutorial/BuildingAJIT2.rst
232

I'm not sure this sentence is actually more clear. You're trying to go from a passive form to an active form, but it goes with introducing someone as the subject, which does not seem like usual to me when describing some piece of code calling another piece of code.
Are you following some guidelines on technical documentation here?

I'm not sure this sentence is actually more clear. You're trying to go from a passive form to an active form, but it goes with introducing someone as the subject, which does not seem like usual to me when describing some piece of code calling another piece of code.
Are you following some guidelines on technical documentation here?

No.

I think it's more clear...
I'm not so sure about that. I'll think about it some more.

dblaikie added a subscriber: dblaikie.

Probably worth including the JIT owner & author (@lhames) in reviews/changes to the JIT documentation.

lhames accepted this revision.Sep 1 2021, 9:53 PM

Very belatedly: These changes look good to me. :)

llvm/docs/tutorial/BuildingAJIT2.rst
232

Agreed -- introducing "someone" here feels slightly awkward. I don't have strong feelings though, and the rest of the changes look good.

This revision is now accepted and ready to land.Sep 1 2021, 9:53 PM

I do not have commit access, I need someone to land it.

Thank you for your review.

xgupta added a subscriber: xgupta.Sep 1 2021, 10:44 PM
xgupta added inline comments.
llvm/docs/tutorial/BuildingAJIT2.rst
232

Yes, someone feels awkward.

sonson added a comment.Sep 2 2021, 1:14 AM
This comment was removed by sonson.
sonson updated this revision to Diff 370306.Sep 2 2021, 9:43 AM

Removed "someone" expression.

I replaced the sentence pointed out by three reviewers with the current one.

Can you rebase? Seems like your patch isn't written on top of main.

sonson updated this revision to Diff 370456.Sep 2 2021, 5:29 PM

Fixed the patch, by rebasing.

mehdi_amini added inline comments.Sep 2 2021, 8:18 PM
llvm/docs/tutorial/BuildingAJIT2.rst
232

I'm not sure remove "the" from "to the layer's emit method" is correct here?

sonson updated this revision to Diff 370475.Sep 2 2021, 9:08 PM

Fixed the patch, by rebasing.
Fixed 'the'.

This revision was landed with ongoing or failed builds.Sep 2 2021, 9:59 PM
This revision was automatically updated to reflect the committed changes.