[PATCH] D56654: Update GettingStarted guide to recommend that people use the new official Git repository.

[James Y Knight via Phabricator via llvm-commits]

jyknight added inline comments.


================
Comment at: llvm/docs/GettingStarted.rst:65
 
+     * ``-DLLVM_ENABLE_PROJECTS='...'`` --- semicolor-separated list of the LLVM
+       subprojects you'd like to additionally build. Can include any of: clang,
----------------
smeenai wrote:
> Do you think LLVM_ENABLE_RUNTIMES is worth mentioning as well, or would that be information overload for a getting started guide?
> 
> Also, typo: semicolor -> semicolon
I don't actually know what that does and when one would want to use it. So either I've been missing out terribly, or else it's not needed for a getting started guide. :)


================
Comment at: llvm/docs/GettingStarted.rst:96
+       ``make -j NNN`` (with an appropriate value of NNN, e.g. number of CPUs
+       you have.)
 
----------------
jlebar wrote:
> or just `make -j`?
"make -j" is bad, it doesn't run number-of-CPUs jobs, instead it puts NO limit on the number of jobs. That usually, IME, eats up all your RAM and destroys your computer.


================
Comment at: llvm/docs/GettingStarted.rst:444
 
-  % git diff --check master..mybranch
+  % git diff --check origin/master..mybranch
 
----------------
jlebar wrote:
> I wonder if now would be a good point to mention clang-format (and the git-clang-format script) instead?  Like, overall that's probably more useful to introduce to beginners than git diff --check.
Seems reasonable.


================
Comment at: llvm/docs/GettingStarted.rst:450
 
-  % git diff master..mybranch > /path/to/mybranch.diff
+  % git diff origin/master..mybranch > /path/to/mybranch.diff
 
----------------
mehdi_amini wrote:
> These instructions assume that "my branch" is perfectly rebased, I think this should use three dots to do what we want: `git diff origin/master...mybranch`
Now that I look over this again, I don't think most of this belongs in here, at all. I rewrote this part to just point people to the Phab instructions, and just left a note that email is also OK.


================
Comment at: llvm/docs/GettingStarted.rst:582
+|                         | include: clang, libcxx, libcxxabi, libunwind, lldb,|
+|                         | compiler-rt, lld, polly, or debuginfo-tests.       |
++-------------------------+----------------------------------------------------+
----------------
jlebar wrote:
> Instead of or in addition to listing them, would it be worthwhile to explain where the canonical list of allowed strings lives (or how to print it)?  That way the list never goes out of date.
I have no idea how to print a list from cmake. Is that even possible? I don't think pointing to the source code would be very helpful, I'd prefer to just have an outdated list vs that.


================
Comment at: llvm/docs/Phabricator.rst:200
 
   git checkout master
   git merge --ff-only arcpatch-D<Revision>
----------------
mehdi_amini wrote:
> jlebar wrote:
> > I would think that you don't want to merge the change into your local "master" branch, because you probably (?) want that to be a copy of origin/master, and upstream is going to rewrite this change.  IOW `git checkout master; git pull --ff-only origin/master` will fail.
> > 
> > Would it make more sense to checkout `arcpatch-D123`, merge origin/master into it, and then `git llvm push` from that branch?
> > 
> > (I'm also confused why we're talking about patching in a change from phab -- isn't the most likely scenario that you already have the change in your git repository?)
> > Would it make more sense to checkout arcpatch-D123, merge origin/master into it, and then git llvm push from that branch?
> 
> My usual flow would be to rebase: `git checkout arcpatch-D123 && git fetch && git rebase origin/master`
I've adjusted these instructions.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D56654/new/

https://reviews.llvm.org/D56654