[llvm] [llvm][Docs] Update MyFirstTypoFix doc (PR #79149)
David Spickett via llvm-commits
llvm-commits at lists.llvm.org
Wed Jan 24 02:03:47 PST 2024
https://github.com/DavidSpickett updated https://github.com/llvm/llvm-project/pull/79149
>From c8b2e7568f7119955a0a504663393b2f04f5e8bc Mon Sep 17 00:00:00 2001
From: David Spickett <david.spickett at linaro.org>
Date: Tue, 23 Jan 2024 13:47:34 +0000
Subject: [PATCH 1/3] [llvm][Docs] Update MyFirstTypoFix doc
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.
---
llvm/docs/CodeOfConduct.rst | 2 +
llvm/docs/CodeReview.rst | 2 +
llvm/docs/DeveloperPolicy.rst | 4 +
llvm/docs/GettingStarted.rst | 2 +
llvm/docs/MyFirstTypoFix.rst | 256 ++++++++++++++++++----------------
5 files changed, 144 insertions(+), 122 deletions(-)
diff --git a/llvm/docs/CodeOfConduct.rst b/llvm/docs/CodeOfConduct.rst
index c88c86369382e84..742fe69cfed3b25 100644
--- a/llvm/docs/CodeOfConduct.rst
+++ b/llvm/docs/CodeOfConduct.rst
@@ -2,6 +2,8 @@
This work is licensed under a Creative Commons Attribution 3.0 Unported License.
SPDX-License-Identifier: CC-BY-3.0
+.. _code_of_conduct:
+
==============================
LLVM Community Code of Conduct
==============================
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 f49b5b13378ea21..644d09702553717 100644
--- a/llvm/docs/DeveloperPolicy.rst
+++ b/llvm/docs/DeveloperPolicy.rst
@@ -1,3 +1,5 @@
+.. _developer_policy:
+
=====================
LLVM Developer Policy
=====================
@@ -468,6 +470,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..a012323164fb324 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]`` 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
-Finding a reviewer
-------------------
+ $ git log --oneline ../clang/
-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 different member of the projects depending on the component you
+notify different 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 <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,10 +398,31 @@ 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 revision
+~~~~~~~~~~~~~~~~~~~~
+
+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 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.
+
Commit access
=============
@@ -439,19 +439,15 @@ about getting commit access yourself. It's probably a good idea if:
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>`__.
+LLVM uses Git for committing changes. The details are 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
@@ -468,32 +464,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**.
+
+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.
-If you see a broken build, do not despair - some build bots are
+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
+differ from what you may be used to elsewhere on GitHub. Within the project
+the expectations of different 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.
>From 7cdd7d1e76d4957d2a9fda75982a7870a453b7d6 Mon Sep 17 00:00:00 2001
From: David Spickett <david.spickett at linaro.org>
Date: Wed, 24 Jan 2024 09:53:58 +0000
Subject: [PATCH 2/3] Missing word in tags section.
---
llvm/docs/MyFirstTypoFix.rst | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/llvm/docs/MyFirstTypoFix.rst b/llvm/docs/MyFirstTypoFix.rst
index a012323164fb324..8d48e9022111479 100644
--- a/llvm/docs/MyFirstTypoFix.rst
+++ b/llvm/docs/MyFirstTypoFix.rst
@@ -307,7 +307,7 @@ For now, we'll only add a title.
Now we're ready to send this change out into the world!
-The ``[clang]`` and ``[Diagnostic]`` are what we call tags. This loose convention
+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.
>From da6cd7cebda389c2f200d6de35b176eda6495884 Mon Sep 17 00:00:00 2001
From: David Spickett <david.spickett at linaro.org>
Date: Wed, 24 Jan 2024 10:03:24 +0000
Subject: [PATCH 3/3] Put the titles in more logical sections.
---
llvm/docs/MyFirstTypoFix.rst | 20 +++++++++-----------
1 file changed, 9 insertions(+), 11 deletions(-)
diff --git a/llvm/docs/MyFirstTypoFix.rst b/llvm/docs/MyFirstTypoFix.rst
index 8d48e9022111479..7b2880c4626064a 100644
--- a/llvm/docs/MyFirstTypoFix.rst
+++ b/llvm/docs/MyFirstTypoFix.rst
@@ -402,8 +402,8 @@ For your first patches, this means:
opinion.
-Accepting a revision
-~~~~~~~~~~~~~~~~~~~~
+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
@@ -411,6 +411,9 @@ 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
---------------
@@ -423,8 +426,8 @@ them! Leave a comment on the review like:
The pull-request will be closed and you will be notified by GitHub.
-Commit access
-=============
+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,12 +439,7 @@ 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
-:ref:`developer policy document <obtaining_commit_access>`.
-
+The process is described in the :ref:`developer policy document <obtaining_commit_access>`.
With great power
----------------
@@ -451,7 +449,7 @@ 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.
More information about the llvm-commits
mailing list