[llvm] ba5d92e - [llvm][Docs] Update MyFirstTypoFix doc (#79149)

via llvm-commits llvm-commits at lists.llvm.org
Mon Jan 29 02:34:27 PST 2024 

Author: David Spickett
Date: 2024-01-29T10:34:22Z
New Revision: ba5d92eb9c18a9037dd06a74f90ceba3f4e3ace9

URL: https://github.com/llvm/llvm-project/commit/ba5d92eb9c18a9037dd06a74f90ceba3f4e3ace9
DIFF: https://github.com/llvm/llvm-project/commit/ba5d92eb9c18a9037dd06a74f90ceba3f4e3ace9.diff

LOG: [llvm][Docs] Update MyFirstTypoFix doc (#79149)

I've not tried to change the purpose or style of the doc, just edited
for clarity and removed any Phabricator related language in favour of
GitHub terms.

Where possible, I've swapped direct links to LLVM's website with RST
links to the local documents. Which should be a bit more resilient.

Also it's less confusing if you're editing multiple pages locally, you
don't accidentally end up on the live site.

Added: 
    

Modified: 
    llvm/docs/CodeReview.rst
    llvm/docs/DeveloperPolicy.rst
    llvm/docs/GettingStarted.rst
    llvm/docs/MyFirstTypoFix.rst

Removed: 
    


################################################################################
diff  --git a/llvm/docs/CodeReview.rst b/llvm/docs/CodeReview.rst
index 5b99f6e3f3f83d0..f1d5b6c9c99206b 100644
--- a/llvm/docs/CodeReview.rst
+++ b/llvm/docs/CodeReview.rst
@@ -1,3 +1,5 @@
+.. _code_review_policy:
+
 =====================================
 LLVM Code-Review Policy and Practices
 =====================================

diff  --git a/llvm/docs/DeveloperPolicy.rst b/llvm/docs/DeveloperPolicy.rst
index 7a7c17ba570047e..d383de58da55a71 100644
--- a/llvm/docs/DeveloperPolicy.rst
+++ b/llvm/docs/DeveloperPolicy.rst
@@ -1,3 +1,5 @@
+.. _developer_policy:
+
 =====================
 LLVM Developer Policy
 =====================
@@ -469,6 +471,8 @@ What are the expectations around a revert?
 * When re-applying a reverted patch, the commit message should be updated to
   indicate the problem that was addressed and how it was addressed.
 
+.. _obtaining_commit_access:
+
 Obtaining Commit Access
 -----------------------
 

diff  --git a/llvm/docs/GettingStarted.rst b/llvm/docs/GettingStarted.rst
index da9cc8aea6d32d8..98d47c945aeee7a 100644
--- a/llvm/docs/GettingStarted.rst
+++ b/llvm/docs/GettingStarted.rst
@@ -330,6 +330,8 @@ Unix utilities. Specifically:
 .. _below:
 .. _check here:
 
+.. _host_cpp_toolchain:
+
 Host C++ Toolchain, both Compiler and Standard Library
 ------------------------------------------------------
 

diff  --git a/llvm/docs/MyFirstTypoFix.rst b/llvm/docs/MyFirstTypoFix.rst
index b8d34733122c114..6d4be73d05681b2 100644
--- a/llvm/docs/MyFirstTypoFix.rst
+++ b/llvm/docs/MyFirstTypoFix.rst
@@ -37,16 +37,14 @@ Clang has a warning for infinite recursion:
 
    $ echo "void foo() { foo(); }" > ~/test.cc
    $ clang -c -Wall ~/test.cc
-   input.cc:1:14: warning: all paths through this function will call
-   itself [-Winfinite-recursion]
+   test.cc:1:12: warning: all paths through this function will call itself [-Winfinite-recursion]
 
 This is clear enough, but not exactly catchy. Let's improve the wording
 a little:
 
 .. code:: console
 
-   input.cc:1:14: warning: to understand recursion, you must first
-   understand recursion [-Winfinite-recursion]
+   test.cc:1:12: warning: to understand recursion, you must first understand recursion [-Winfinite-recursion]
 
 
 Dependencies
@@ -57,20 +55,19 @@ We're going to need some tools:
 -  git: to check out the LLVM source code,
 
 -  a C++ compiler: to compile LLVM source code. You'll want `a recent
-   version <https://llvm.org/docs/GettingStarted.html#host-c-toolchain-both-compiler-and-standard-library>`__
-   of Clang, GCC, or Visual Studio.
+   version <host_cpp_toolchain>` of Clang, GCC, or Visual Studio.
 
 -  CMake: used to configure how LLVM should be built on your system,
 
 -  ninja: runs the C++ compiler to (re)build specific parts of LLVM,
 
--  python: to run the LLVM tests,
+-  python: to run the LLVM tests.
 
 As an example, on Ubuntu:
 
 .. code:: console
 
-   $ sudo apt-get install git clang cmake ninja-build python arcanist
+   $ sudo apt-get install git clang cmake ninja-build python
 
 
 Building LLVM
@@ -106,8 +103,7 @@ by running CMake. CMake combines information from three sources:
 
 -  project structure (which files are part of 'clang'?)
 
-First, create a directory to build in. Usually, this is
-llvm-project/build.
+First, create a directory to build in. Usually, this is ``llvm-project/build``.
 
 .. code:: console
 
@@ -129,12 +125,12 @@ finally:
    Generating done
    Build files have been written to: /path/llvm-project/build
 
-And you should see a build.ninja file.
+And you should see a ``build.ninja`` file in the current directory.
 
 Let's break down that last command a little:
 
--  **-G Ninja**: we're going to use ninja to build; please create
-   build.ninja
+-  **-G Ninja**: Tells CMake that we're going to use ninja to build, and to create
+   the ``build.ninja`` file.
 
 -  **../llvm**: this is the path to the source of the "main" LLVM
    project
@@ -148,18 +144,17 @@ Let's break down that last command a little:
        If you want to run under a debugger, you should use the default Debug
        (which is totally unoptimized, and will lead to >10x slower test
        runs) or RelWithDebInfo which is a halfway point.
-       **CMAKE_BUILD_TYPE** affects code generation only, assertions are
-       on by default regardless! **LLVM_ENABLE_ASSERTIONS=Off** disables
-       them.
+
+       Assertions are not enabled in ``Release`` builds by default.
+       You can enable them using ``LLVM_ENABLE_ASSERTIONS=ON``.
 
     -  **LLVM_ENABLE_PROJECTS=clang**: this lists the LLVM subprojects
        you are interested in building, in addition to LLVM itself. Multiple
-       projects can be listed, separated by semicolons, such as "clang;
-       lldb".In this example, we'll be making a change to Clang, so we
-       should build it.
+       projects can be listed, separated by semicolons, such as ``clang;lldb``.
+       In this example, we'll be making a change to Clang, so we only add clang.
 
-Finally, create a symlink (or a copy) of
-llvm-project/build/compile-commands.json into llvm-project/:
+Finally, create a symlink (or copy) of ``llvm-project/build/compile-commands.json``
+into ``llvm-project/``:
 
 .. code:: console
 
@@ -197,16 +192,16 @@ There's also a target for building and running all the clang tests:
 
    $ ninja check-clang
 
-This is a common pattern in LLVM: check-llvm is all the checks for core,
-other projects have targets like check-lldb.
+This is a common pattern in LLVM: check-llvm is all the checks for the core of
+LLVM, other projects have targets like ``check-lldb``, ``check-flang`` and so on.
 
 
 Making changes
 ==============
 
 
-Edit
-----
+The Change
+----------
 
 We need to find the file containing the error message.
 
@@ -215,8 +210,8 @@ We need to find the file containing the error message.
    $ git grep "all paths through this function" ..
    ../clang/include/clang/Basic/DiagnosticSemaKinds.td:  "all paths through this function will call itself">,
 
-The string that appears in DiagnosticSemaKinds.td is the one that is
-printed by Clang. \*.td files define tables - in this case it's a list
+The string that appears in ``DiagnosticSemaKinds.td`` is the one that is
+printed by Clang. ``*.td`` files define tables - in this case it's a list
 of warnings and errors clang can emit and their messages. Let's update
 the message in your favorite editor:
 
@@ -224,9 +219,8 @@ the message in your favorite editor:
 
    $ vi ../clang/include/clang/Basic/DiagnosticSemaKinds.td
 
-Find the message (it should be under
-``warn_infinite_recursive_function``). Change the message to "in order to
-understand recursion, you must first understand recursion".
+Find the message (it should be under ``warn_infinite_recursive_function``).
+Change the message to "in order to understand recursion, you must first understand recursion".
 
 
 Test again
@@ -238,9 +232,8 @@ works.
 .. code:: console
 
    $ ninja clang
-   $ bin/clang -Wall ~/test.cc
-   /path/test.cc:1:124: warning: in order to understand recursion, you must
-   first understand recursion [-Winfinite-recursion]
+   $ bin/clang -c -Wall ~/test.cc
+   test.cc:1:12: warning: in order to understand recursion, you must first understand recursion [-Winfinite-recursion]
 
 We should also run the tests to make sure we didn't break something.
 
@@ -254,16 +247,14 @@ so it runs them all.
 
 .. code:: console
 
-   ********************
-   Testing Time: 408.84s
    ********************
    Failing Tests (1):
        Clang :: SemaCXX/warn-infinite-recursion.cpp
 
 Well, that makes sense… and the test output suggests it's looking for
 the old string "call itself" and finding our new message instead.
-Note that more tests may fail in a similar way as new tests are
-added time to time.
+Note that more tests may fail in a similar way as new tests are added
+over time.
 
 Let's fix it by updating the expectation in the test.
 
@@ -271,33 +262,32 @@ Let's fix it by updating the expectation in the test.
 
    $ vi ../clang/test/SemaCXX/warn-infinite-recursion.cpp
 
-Everywhere we see `// expected-warning{{call itself}}` (or something similar
+Everywhere we see ``// expected-warning{{call itself}}`` (or something similar
 from the original warning text), let's replace it with
-`// expected-warning{{to understand recursion}}`.
+``// expected-warning{{to understand recursion}}``.
 
 Now we could run **all** the tests again, but this is a slow way to
 iterate on a change! Instead, let's find a way to re-run just the
 specific test. There are two main types of tests in LLVM:
 
--  **lit tests** (e.g. SemaCXX/warn-infinite-recursion.cpp).
+-  **lit tests** (e.g. ``SemaCXX/warn-infinite-recursion.cpp``).
 
 These are fancy shell scripts that run command-line tools and verify the
 output. They live in files like
-clang/**test**/FixIt/dereference-addressof.c. Re-run like this:
+``clang/**test**/FixIt/dereference-addressof.c``. Re-run like this:
 
 .. code:: console
 
    $ bin/llvm-lit -v ../clang/test/SemaCXX/warn-infinite-recursion.cpp
 
--  **unit tests** (e.g. ToolingTests/ReplacementTest.CanDeleteAllText)
+-  **unit tests** (e.g. ``ToolingTests/ReplacementTest.CanDeleteAllText``)
 
 These are C++ programs that call LLVM functions and verify the results.
 They live in suites like ToolingTests. Re-run like this:
 
 .. code:: console
 
-   $ ninja ToolingTests && tools/clang/unittests/Tooling/ToolingTests
-   --gtest_filter=ReplacementTest.CanDeleteAllText
+   $ ninja ToolingTests && tools/clang/unittests/Tooling/ToolingTests --gtest_filter=ReplacementTest.CanDeleteAllText
 
 
 Commit locally
@@ -305,97 +295,85 @@ Commit locally
 
 We'll save the change to a local git branch. This lets us work on other
 things while the change is being reviewed. Changes should have a
-description, to explain to reviewers and future readers of the code why
+title and description, to explain to reviewers and future readers of the code why
 the change was made.
 
+For now, we'll only add a title.
+
 .. code:: console
 
    $ git checkout -b myfirstpatch
-   $ git commit -am "[Diagnostic] Clarify -Winfinite-recursion message"
+   $ git commit -am "[clang][Diagnostic] Clarify -Winfinite-recursion message"
 
-Now we're ready to send this change out into the world! By the way,
-There is an unwritten convention of using tag for your commit. Tags
-usually represent modules that you intend to modify. If you don't know
-the tags for your modules, you can look at the commit history :
-https://github.com/llvm/llvm-project/commits/main.
+Now we're ready to send this change out into the world!
 
+The ``[clang]`` and ``[Diagnostic]`` prefixes are what we call tags. This loose convention
+tells readers of the git log what areas a change is modifying. If you don't know
+the tags for the modules you've changed, you can look at the commit history
+for those areas of the repository.
 
-Code review
-===========
+.. code:: console
 
+   $ git log --oneline ../clang/
 
-Finding a reviewer
-------------------
-
-Changes can be reviewed by anyone in the LLVM community who has commit
-access.For larger and more complicated changes, it's important that the
-reviewer has experience with the area of LLVM and knows the design goals
-well. The author of a change will often assign a specific reviewer (git
-blame and git log can be useful to find one).
+Or using GitHub, for example https://github.com/llvm/llvm-project/commits/main/clang.
 
-As our change is fairly simple, we'll add the cfe-commits mailing list
-as a subscriber; anyone who works on clang can likely pick up the
-review. (For changes outside clang, llvm-commits is the usual list. See
-`http://lists.llvm.org/ <http://lists.llvm.org/mailman/listinfo>`__ for
-all the \*-commits mailing lists).
+Tagging is imprecise, so don't worry if you are not sure what to put. Reviewers
+will suggest some if they think they are needed.
 
+Code review
+===========
 
 Uploading a change for review
 -----------------------------
 
-LLVM code reviews happen through pull-request on GitHub, see
+LLVM code reviews happen through pull-request on GitHub, see the
 :ref:`GitHub <github-reviews>` documentation for how to open
 a pull-request on GitHub.
 
+Finding a reviewer
+------------------
+
+Changes can be reviewed by anyone in the LLVM community. For larger and more
+complicated changes, it's important that the
+reviewer has experience with the area of LLVM and knows the design goals
+well. The author of a change will often assign a specific reviewer. ``git blame``
+and ``git log`` can be useful to find previous authors who can review.
+
+Our GitHub bot will also tag and notify various "teams" around LLVM. The
+team members contribute and review code for those specific areas regularly,
+so one of them will review your change if you don't pick anyone specific.
+
 Review process
 --------------
 
 When you open a pull-request, some automation will add a comment and
-notify 
diff erent member of the projects depending on the component you
+notify 
diff erent members of the sub-projects depending on the parts you have
 changed.
+
 Within a few days, someone should start the review. They may add
 themselves as a reviewer, or simply start leaving comments. You'll get
-another email any time the review is updated. The details are in the
-`https://llvm.org/docs/CodeReview/ <https://llvm.org/docs/CodeReview.html>`__.
+another email any time the review is updated. For more detail see the
+:ref:`Code Review Poilicy <code_review_policy>`.
 
 Comments
 ~~~~~~~~
 
 The reviewer can leave comments on the change, and you can reply. Some
 comments are attached to specific lines, and appear interleaved with the
-code. You can either reply to these, or address them and mark them as
-"done". Note that in-line replies are **not** sent straight away! They
-become "draft" comments and you must click "Submit" at the bottom of the
-page.
-
+code. You can reply to these. Perhaps to clarify what was asked or to tell the
+reviewer that you have done what was asked.
 
 Updating your change
 ~~~~~~~~~~~~~~~~~~~~
 
 If you make changes in response to a reviewer's comments, simply update
-your branch with more commits and push to your fork. It may be a good
-idea to answer the comments from the reviewer explicitly.
-
-Accepting a revision
-~~~~~~~~~~~~~~~~~~~~
-
-When the reviewer is happy with the change, they will **Accept** the
-revision. They may leave some more minor comments that you should
-address, but at this point the review is complete. It's time to get it
-merged!
+your branch with more commits and push to your GitHub fork of ``llvm-project``.
+It is best if you answer comments from the reviewer directly instead of expecting
+them to read through all the changes again.
 
-
-Commit by proxy
----------------
-
-As this is your first change, you won't have access to merge it
-yourself yet. The reviewer **doesn't know this**, so you need to tell
-them! Leave a message on the review like:
-
-   Thanks @somellvmdev. I don't have commit access, can you land this
-   patch for me?
-
-The pull-request will be closed and you will be notified by GitHub.
+For example you might comment "I have done this." or "I was able to this part
+but have a question about...".
 
 Review expectations
 -------------------
@@ -408,8 +386,9 @@ overall architecture of the project.
 
 For your first patches, this means:
 
--  be kind, and expect reviewers to be kind in return - LLVM has a `Code
-   of Conduct <https://llvm.org/docs/CodeOfConduct.html>`__;
+-  be kind, and expect reviewers to be kind in return - LLVM has a
+   :ref:`Code of Conduct <LLVM Community Code of Conduct>`
+   that everyone should be following;
 
 -  be patient - understanding how a new feature fits into the
    architecture of the project is often a time consuming effort, and
@@ -419,13 +398,37 @@ For your first patches, this means:
 -  if you can't agree, generally the best way is to do what the reviewer
    asks; we optimize for readability of the code, which the reviewer is
    in a better position to judge; if this feels like it's not the right
-   option, you can contact the cfe-dev mailing list to get more feedback
-   on the direction;
+   option, you can ask them in a comment or add another reviewer to get a second
+   opinion.
+
+
+Accepting a pull request
+------------------------
+
+When the reviewer is happy with the change, they will **Approve** the
+pull request. They may leave some more minor comments that you should
+address before it is merged, but at this point the review is complete.
+It's time to get it merged!
 
 
 Commit access
 =============
 
+Commit by proxy
+---------------
+
+As this is your first change, you won't have access to merge it
+yourself yet. The reviewer **does not know this**, so you need to tell
+them! Leave a comment on the review like:
+
+   Thanks @<username of reviewer>. I don't have commit access, can you merge this
+   PR for me?
+
+The pull-request will be closed and you will be notified by GitHub.
+
+Getting commit access
+---------------------
+
 Once you've contributed a handful of patches to LLVM, start to think
 about getting commit access yourself. It's probably a good idea if:
 
@@ -436,26 +439,17 @@ about getting commit access yourself. It's probably a good idea if:
 -  you'd like to keep contributing to LLVM.
 
 
-Getting commit access
----------------------
-
-LLVM uses Git for committing changes. The details are in the `developer
-policy
-document <https://llvm.org/docs/DeveloperPolicy.html#obtaining-commit-access>`__.
-
+The process is described in the :ref:`developer policy document <obtaining_commit_access>`.
 
 With great power
 ----------------
 
-Actually, this would be a great time to read the rest of the `developer
-policy <https://llvm.org/docs/DeveloperPolicy.html>`__, too. At minimum,
-you need to be subscribed to the relevant commits list before landing
-changes (e.g. llvm-commits at lists.llvm.org), as discussion often happens
-there if a new patch causes problems.
+Actually, this would be a great time to read the rest of the :ref:`developer
+policy <developer_policy>` too.
 
 
 Post-commit errors
-------------------
+==================
 
 Once your change is submitted it will be picked up by automated build
 bots that will build and test your patch in a variety of configurations.
@@ -468,32 +462,48 @@ which configuration have been broken for a while.
 The console view at http://lab.llvm.org/buildbot/#/console helps to get a
 better understanding of the build results of a specific patch. If you
 want to follow along how your change is affecting the build bots, **this
-should be the first place to look at** - the colored bubbles correspond
-to projects in the waterfall.
+should be the first place to look at**.
 
-If you see a broken build, do not despair - some build bots are
+Note: only recent changes are shown in the console view. So if your change
+is not there, please rely on PR comments and buildbot emails to notify you
+of any issues intead.
+
+The colored bubbles correspond to projects in the waterfall. If you see a broken
+build, do not despair - some build bots are
 continuously broken; if your change broke the build, you will see a red
 bubble in the console view, while an already broken build will show an
 orange bubble. Of course, even when the build was already broken, a new
 change might introduce a hidden new failure.
 
-| When you want to see more details how a specific build is broken,
-  click the red bubble.
-| If post-commit error logs confuse you, do not worry too much -
-  everybody on the project is aware that this is a bit unwieldy, so
-  expect people to jump in and help you understand what's going on!
-
-buildbots, overview of bots, getting error logs.
+When you want to see more details how a specific build is broken, click the red bubble.
+If the error logs confuse you, do not worry - you can ask for help by adding a comment
+to your PR, or asking on `Discord <https://discord.com/invite/xS7Z362>`__.
 
 
 Reverts
 -------
 
-If in doubt, revert immediately, and re-land later after investigation
-and fix.
+If your change has caused a problem, it should be reverted as soon as possible.
+This is a normal part of :ref:`LLVM development <revert_policy>`,
+that every committer (no matter how experienced) goes through.
 
+If you are in any doubt whether your change can be fixed quickly, revert it.
+Then you have plenty of time to investigate and produce a solid fix.
+
+Someone else may revert your change for you, or you can create a revert pull request using
+the `GitHub interface <https://docs.github.com/en/pull-requests/collaborating-with-pull-requests/incorporating-changes-from-a-pull-request/reverting-a-pull-request>`__.
+Add your original reviewers to this new pull request if possible.
 
 Conclusion
 ==========
 
-llvm is a land of contrasts.
+Now you should have an understanding of the life cycle of a contribution to the
+LLVM Project.
+
+If some details are still unclear, do not worry. The LLVM Project's process does
+
diff er from what you may be used to elsewhere on GitHub. Within the project
+the expectations of 
diff erent sub-projects may vary too.
+
+So whatever you are contributing to, know that we are not expecting perfection.
+Please ask questions whenever you are unsure, and expect that if you have missed
+something, someone will politely point it out and help you address it.

More information about the llvm-commits mailing list