-
Notifications
You must be signed in to change notification settings - Fork 15.2k
[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 2 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,76 @@ | ||
| --- | ||
| 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. | ||
|
Collaborator
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?
Member
Author
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. |
||
|
|
||
| ## 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. | ||
| - Avoid static constructors or global objects with heavy initialization. | ||
|
||
| - Use `class` or `struct` consistently; `struct` only for all-public data. | ||
|
Collaborator
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?
Collaborator
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 ;) )
Member
Author
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! |
||
|
|
||
| ## 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. | ||
| - Avoid `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. | ||
| - Avoid `using namespace std;` in headers. | ||
|
||
| - Ensure at least one out-of-line virtual method per class with virtuals. | ||
|
||
| - For `switch` on enums, omit `default` to catch missing cases. | ||
| - Prefer range-based `for` loops. | ||
| - Capture `end()` outside loops if not using range-based iteration. | ||
| - Use LLVM’s `raw_ostream` instead of `<iostream>`. | ||
| - Avoid `std::endl`; use `\n` unless flushing is needed. | ||
| - Methods in class definitions are already inline—don’t add `inline`. | ||
|
|
||
| ## Microscopic Details | ||
|
|
||
|
Collaborator
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. | ||
| - 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 Standars: https://llvm.org/docs/CodingStandards.html. | ||
| - Ask clarifying questions when code intent is unclear. | ||
|
|
||
| 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"