-
Notifications
You must be signed in to change notification settings - Fork 15.1k
[GitHub] Add Copilot review instructions for LLDB #165783
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,79 @@ | ||
| --- | ||
| applyTo: lldb/**/* | ||
| --- | ||
|
|
||
| When reviewing code, focus on: | ||
|
|
||
| ## Language, Libraries & Standards | ||
|
|
||
| - Target C++17 and avoid vendor-specific extensions. | ||
| - For Python scripts, follow PEP 8. | ||
| - Prefer standard library or LLVM support libraries instead of reinventing data structures. | ||
|
|
||
| ## Comments & Documentation | ||
|
|
||
| - Each source file should include the standard LLVM file header. | ||
| - Header files must have proper header guards. | ||
| - Non-trivial classes and public methods should have Doxygen documentation. | ||
| - Use `//` or `///` comments normally; avoid block comments unless necessary. | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. By the point I got to this point (reading or Coding Standards document in parallel), I'm guessing you're leaving out everything that is checked/covered by clang-format. Is that correct? If so, maybe that's a "design decision" that would be good to document at least in the commit message? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes, I want to leave all that up to clang-format. I'll include an explicit directive to ignore it. |
||
| - Non-trivial code should have comments explaining what it does and why. Avoid comments that explain how it does it at a micro level. | ||
|
|
||
| ## Language & Compiler Issues | ||
|
|
||
| - Write portable code; wrap non-portable code in interfaces. | ||
| - Do not use RTTI or exceptions. | ||
| - Prefer C++-style casts over C-style casts. | ||
| - Do not use static constructors. | ||
| - Use `class` or `struct` consistently; `struct` only for all-public data. | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I found this sentence a bit too cryptic for me too understand what was meant. Maybe that makes it too cryptic for copilot too?
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. (BTW, this is where I'm starting to suspect you might have used the help of an LLM to produce this ;) ) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I absolutely did! |
||
| - When then same class is declared or defined multiple times, make sure it's consistently done using either `class` or `struct`. | ||
|
|
||
| ## Headers & Library Layering | ||
|
|
||
| - Include order: module header → local/private headers → project headers → system headers. | ||
| - Headers must compile standalone (include all dependencies). | ||
| - Maintain proper library layering; avoid circular dependencies. | ||
| - Include minimally; use forward declarations where possible. | ||
| - Keep internal headers private to modules. | ||
| - Use full namespace qualifiers for out-of-line definitions. | ||
|
|
||
| ## Control Flow & Structure | ||
|
|
||
| - Prefer early exits over deep nesting. | ||
| - Do not use `else` after `return`, `continue`, `break`, or `goto`. | ||
| - Encapsulate loops that compute predicates into helper functions. | ||
|
|
||
| ## Naming | ||
|
|
||
| - LLDB's code style differs from LLVM's coding style. | ||
| - Variables are `snake_case`. | ||
| - Functions and methods are `UpperCamelCase`. | ||
| - Static, global and member variables have `s_`, `g_` and `m_` prefixes respectively. | ||
|
|
||
| ## General Guidelines | ||
|
|
||
| - Use `assert` liberally; prefer `llvm_unreachable` for unreachable states. | ||
| - Do not use `using namespace std;` in headers. | ||
| - Provide a virtual method anchor for classes defined in headers. | ||
| - Do not use default labels in fully covered switches over enumerations. | ||
| - Use range-based for loops wherever possible. | ||
| - Capture `end()` outside loops if not using range-based iteration. | ||
| - Including `<iostream>` is forbidded. Use LLVM’s `raw_ostream` instead. | ||
| - Don’t use `inline` when defining a function in a class definition. | ||
|
|
||
| ## Microscopic Details | ||
|
|
||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Reading through the coding standard in parallel, maybe this section should also have: |
||
| - Preserve existing style in modified code. | ||
| - Prefer pre-increment (`++i`) when value is unused. | ||
| - Use `private`, `protected`, or `public` keyword as appropriate to restrict class member visibility. | ||
| - Omit braces for single-statement `if`, `else`, `while`, `for` unless needed. | ||
|
|
||
| ## Review Style | ||
|
|
||
| - Be specific and actionable in feedback. | ||
| - Explain the "why" behind recommendations. | ||
| - Link back to the LLVM Coding Standards: https://llvm.org/docs/CodingStandards.html. | ||
| - Ask clarifying questions when code intent is unclear. | ||
|
|
||
| Ignore formatting and assume that's handled by external tools like `clang-format` and `black`. | ||
| Remember that these standards are **guidelines**. | ||
| Always prioritize consistency with the style that is already being used by the surrounding code. | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It might be useful to add something here to cover "Aim to describe what the code is trying to do and why, not how it does it at a micro level" from our CodingStandards. Basically, I'd like to say "make sure that non-obvious code has comments explaining what it is aiming to do and why"