diff --git a/llvm/docs/CodeReview.rst b/llvm/docs/CodeReview.rst index 5b99f6e3f3f83..f1d5b6c9c9920 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 7a7c17ba57004..d383de58da55a 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 da9cc8aea6d32..98d47c945aeee 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 b8d34733122c1..6d4be73d05681 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 `__ - of Clang, GCC, or Visual Studio. + version ` 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/ `__ 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 ` 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/ `__. +another email any time the review is updated. For more detail see the +:ref:`Code Review Poilicy `. 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 `__; +- be kind, and expect reviewers to be kind in return - LLVM has a + :ref:`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 @. 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 `__. - +The process is described in the :ref:`developer policy document `. With great power ---------------- -Actually, this would be a great time to read the rest of the `developer -policy `__, too. At minimum, -you need to be subscribed to the relevant commits list before landing -changes (e.g. llvm-commits@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 ` 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 `__. 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 `, +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 `__. +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.