This is an archive of the discontinued LLVM Phabricator instance.

[docs] Update the llvm/example section
ClosedPublic

Authored by pooja2299 on Apr 26 2021, 4:42 AM.

Details

Summary

Added details about the llvm/example section.

Diff Detail

Event Timeline

pooja2299 requested review of this revision.Apr 26 2021, 4:42 AM
pooja2299 created this revision.
Herald added a project: Restricted Project. · View Herald TranscriptApr 26 2021, 4:42 AM
pooja2299 updated this revision to Diff 341251.Apr 28 2021, 9:53 AM

Improved llvm/example section

Removed ExceptionDemo and brainF topic

xgupta accepted this revision.Apr 28 2021, 10:51 AM
xgupta added subscribers: fhahn, reames.

LGTM, Thanks for the patch!

Also adding @fhahn and @reames as reviewers as it is an important doc of LLVM.

This revision is now accepted and ready to land.Apr 28 2021, 10:51 AM

Minor optional suggestions, but LGTM to me too.

@xgupta - I suggest avoiding both LGTMing and asking for another reviewer. Given LLVM is a single accept culture, this can be really confusing to new contributors. I suggest wording along the lines of "This seems good to me, but I'd like a second opinion before LGTMing. Maybe X and Y can confirm?" Alternatively, you could simply LGTM if you were comfortable with that. Another phrasing might be "LGTM, but please wait N days before submitting in case X or Y have additional comments".

llvm/docs/GettingStarted.rst
820–834

The last part of this sentence gets a bit complicated, can I suggest:

Some simple examples showing how to use LLVM as a compiler for a custom language - including lowering, optimization, and code generation.

826

Maybe replace "both static and JIT" with " both static (ahead of time) and various approaches to Just In Time (JIT) compilation."

Minor optional suggestions, but LGTM to me too.

@xgupta - I suggest avoiding both LGTMing and asking for another reviewer. Given LLVM is a single accept culture, this can be really confusing to new contributors. I suggest wording along the lines of "This seems good to me, but I'd like a second opinion before LGTMing. Maybe X and Y can confirm?" Alternatively, you could simply LGTM if you were comfortable with that. Another phrasing might be "LGTM, but please wait N days before submitting in case X or Y have additional comments".

Yes @reames, I agree, I will remember your suggestion from next time. I was just a little afraid that I am also not very experienced so giving only my acceptance to commit changes is a little risky(IMO). If next time I have confusion I only add an appropriate reviewer for that patch.

( Additional context here is that @pooja2299 is an Outreachy applicant and I am co-mentor for one project, so she adds me on every review so I will aware of her progress but I may not be the perfect reviewer for every patch)

pooja2299 updated this revision to Diff 341304.Apr 28 2021, 1:37 PM

Edited few lines as suggested in comments.

@fhahn and @reames can you please review this patch?

reames added a comment.May 5 2021, 8:49 AM

@fhahn and @reames can you please review this patch?

I have already given an LGTM, what more are you expecting?

@fhahn and @reames can you please review this patch?

I have already given an LGTM, what more are you expecting?

I meant after the changes that you asked .
Thanks a lot 😄 👍

This revision was landed with ongoing or failed builds.May 5 2021, 9:04 AM
This revision was automatically updated to reflect the committed changes.
reames added a comment.May 5 2021, 9:05 AM

@fhahn and @reames can you please review this patch?

I have already given an LGTM, what more are you expecting?

I meant after the changes that you asked .
Thanks a lot 😄 👍

Ah, sorry, I'm so used to LLVM phrasing that I forgot "LGTM w/minor optional comments" isn't widely understood. For us, that generally means you can land with the changes made (if you decide to) without further review.

@fhahn and @reames can you please review this patch?

I have already given an LGTM, what more are you expecting?

I meant after the changes that you asked .
Thanks a lot 😄 👍

Ah, sorry, I'm so used to LLVM phrasing that I forgot "LGTM w/minor optional comments" isn't widely understood. For us, that generally means you can land with the changes made (if you decide to) without further review.

Oh ok 👍 I will remember this next time.