Skip to content

[lldb][Docs] Document our major differences from the LLVM style #66345

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 2 commits into from
Sep 19, 2023

Conversation

DavidSpickett
Copy link
Collaborator

@DavidSpickett DavidSpickett commented Sep 14, 2023

Running:
$ clang-format -i $(find -regex "./lldb/..c") $(find -regex "./lldb/..cpp") $(find -regex "./lldb/.*.h")

Resulted in:
1602 files changed, 25090 insertions(+), 25849 deletions(-)
(note: this includes tests which we wouldn't format, just using this as an example)

The vast majority of which were whitespace changes. So as far as formatting we're not deviating from llvm for any reason other than not churning old code.

Formatting aside, the major features of lldb (single line if, early return) are all reflected in llvm's style. We differ mainly on variable naming (proposed to change in https://llvm.org/docs/Proposals/VariableNames.html anyway) and use of asserts. Which was already documented.

@DavidSpickett DavidSpickett requested review from JDevlieghere, bulbazord and a team September 14, 2023 09:10
@llvmbot llvmbot added the lldb label Sep 14, 2023
@llvmbot
Copy link
Member

llvmbot commented Sep 14, 2023

@llvm/pr-subscribers-lldb

Changes Running: $ clang-format -i $(find -regex "\./lldb/.*\.c") $(find -regex "\./lldb/.*\.cpp") $(find -regex "\./lldb/.*\.h")

Resulted in:
1602 files changed, 25090 insertions(+), 25849 deletions(-)
(note: this includes tests which we wouldn't format, just using this as an example)

The vast majority of which were whitespace changes. So as far as formatting we're not deviating from llvm for any reason other than not churning old code.

Formatting aside, the major features of lldb (single line if, early return) are all reflected in llvm's stype. We differ mainly on variable naming (proposed to change in https://llvm.org/docs/Proposals/VariableNames.html anyway) and use of asserts. Which was already documented.

Full diff: https://github.com/llvm/llvm-project/pull/66345.diff

1 Files Affected:

  • (modified) lldb/docs/resources/contributing.rst (+29-10)
diff --git a/lldb/docs/resources/contributing.rst b/lldb/docs/resources/contributing.rst
index 54917f1ce8175b3..9b133706a7fd160 100644
--- a/lldb/docs/resources/contributing.rst
+++ b/lldb/docs/resources/contributing.rst
@@ -18,19 +18,38 @@ Please refer to the `LLVM Developer Policy
 authoring and uploading a patch. LLDB differs from the LLVM Developer
 Policy in the following respects.
 
- - **Test infrastructure**: Like LLVM it is  important to submit tests with your
-   patches, but note that LLDB uses a different system for tests. Refer to the
-   `test documentation <test.html>`_ for more details and the ``lldb/test``
-   folder on disk for examples.
-
- - **Coding Style**: LLDB's code style differs from
-   `LLVM's coding style <https://llvm.org/docs/CodingStandards.html>`_.
-   Unfortunately there is no document describing the differences. Please be
-   consistent with the existing code.
-
 For anything not explicitly listed here, assume that LLDB follows the LLVM
 policy.
 
+Coding Style
+++++++++++++
+
+LLDB's code style differs from `LLVM's coding style <https://llvm.org/docs/CodingStandards.html>`_
+in a few ways. The 2 main ones are:
+
+* `Variable naming <https://llvm.org/docs/CodingStandards.html#name-types-functions-variables-and-enumerators-properly>`_:
+  LLDB prefers variables to be ``named_like_this`` and uses the ``m_`` prefix for
+  member variables.
+
+* `Use of asserts <https://llvm.org/docs/CodingStandards.html#assert-liberally>`_:
+  See the :ref:`section below<Error Handling>`.
+
+For any other contradications please follow the existing code's style.
+
+Code in LLDB does aim to conform to clang-format but older code may not yet. As
+always, consider the `golden rule <https://llvm.org/docs/CodingStandards.html#introduction>`_
+when working with such code. Reformatting before starting work is one possible
+option.
+
+Test Infrastructure
++++++++++++++++++++
+
+Like LLVM it is  important to submit tests with your patches, but note that LLDB
+uses a different system for tests. Refer to the `test documentation <test.html>`_
+for more details and the `lldb/test <https://github.com/llvm/llvm-project/tree/main/lldb/test>`_
+folder for examples.
+
+.. _Error handling:
 
 Error handling and use of assertions in LLDB
 --------------------------------------------

@DavidSpickett
Copy link
Collaborator Author

@bulbazord This would be a good place to put your plugin dependency / const string guidance. Or tell me where it lives now and I'll link to it.

@DavidSpickett
Copy link
Collaborator Author

If anyone knows the other big differences I'll add them. This is what I came up with from my experience and reading the lllvm style doc.

Running:
$ clang-format -i $(find -regex "\./lldb/.*\.c") $(find -regex "\./lldb/.*\.cpp") $(find -regex "\./lldb/.*\.h")

Resulted in:
 1602 files changed, 25090 insertions(+), 25849 deletions(-)
(note: this includes tests which we wouldn't format, just
using this as an example)

The vast majority of which were whitespace changes. So as far
as formatting we're not deviating from llvm for any reason
other than not churning old code.

Formatting aside, the major features of lldb (single line if,
early return) are all reflected in llvm's style. We differ mainly
on variable naming (proposed to change in https://llvm.org/docs/Proposals/VariableNames.html
anyway) and use of asserts. Which was already documented.
Copy link
Member

@bulbazord bulbazord left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Excellent write up, thanks for taking care of that!

@bulbazord
Copy link
Member

@bulbazord This would be a good place to put your plugin dependency / const string guidance. Or tell me where it lives now and I'll link to it.

Oh, yes! I'd be happy to take care of that after this goes in, thanks for pointing that out! :)

* Moved lldbassert() deprecation to a note.
* Show variable naming formats using the name of the format e.g. snake_case.
* Note other variable prefixes.
* Strengthen statement on clang-format.
* Only refer to API tests in test section.
Copy link
Member

@JDevlieghere JDevlieghere left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for working on this!

@DavidSpickett DavidSpickett merged commit ca723f2 into llvm:main Sep 19, 2023
@DavidSpickett DavidSpickett deleted the lldb-style-docs branch September 19, 2023 07:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants