[GitHub] Add Copilot review instructions for LLDB #165783
Conversation
This is an experiment to encode the LLVM Coding Standards [1] as instructions for the Copilot reviewer on GitHub. Ideally, this will catch common issues automatically and reduce the review burden. Adding Copilot as a reviewer is entirely opt-in. Initially, I will add it as a reviewer to test this. If the experiment is successful, we can explore how to integrate this into other parts of LLVM. [1]: https://llvm.org/docs/CodingStandards.html
There was a problem hiding this comment.
Thank you for experimenting with this!
I think it's great to experiment with using LLM tools in ways that help us reduce our limited reviewer bandwidth problem, rather than making it worse!
I left a bunch of nit-picky observations.
But I think this is good to commit as is too, if you think none of those nit-picks make sense.
| ## Comments & Documentation | ||
|
|
||
| - Each source file should include the standard LLVM file header. | ||
| - Header files must have proper header guards. |
There was a problem hiding this comment.
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"
| - 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.
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?
Would it make sense to also have that in this instructions.md file at the top?
There was a problem hiding this comment.
Yes, I want to leave all that up to clang-format. I'll include an explicit directive to ignore it.
| - 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. |
There was a problem hiding this comment.
nit pick: our coding standard says "do not use static constructers" rather than "avoid ...". Not sure if it makes a difference to copilot....
| - 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. |
There was a problem hiding this comment.
I found this sentence a bit too cryptic for me too understand what was meant. Maybe that makes it too cryptic for copilot too?
My attempt to rewrite it to make it less cryptic:
- For classes with only public members, use
structinstead ofclass. - When then same class is declared or defined multiple times, make sure it's consistently done using either
classorstruct.
There was a problem hiding this comment.
(BTW, this is where I'm starting to suspect you might have used the help of an LLM to produce this ;) )
| ## General Guidelines | ||
|
|
||
| - Use `assert` liberally; prefer `llvm_unreachable` for unreachable states. | ||
| - Avoid `using namespace std;` in headers. |
|
|
||
| - 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. |
There was a problem hiding this comment.
s/virtuals./virtual methods./
(Not sure if that would be better. As a human, that reads better, but maybe the shorter form is better for copilot?)
| - Methods in class definitions are already inline—don’t add `inline`. | ||
|
|
||
| ## Microscopic Details | ||
|
|
There was a problem hiding this comment.
Reading through the coding standard in parallel, maybe this section should also have:
"- Functions and variables should have the most restricted visibility possible."
?
|
Thanks Kristof, really appreciate your input. I've addressed your remarks and updated some other places in the same spirit. |
2 participants
This is an experiment to encode the LLVM Coding Standards 1 as instructions for the Copilot reviewer on GitHub. Ideally, this will catch common issues automatically and reduce the review burden.
Adding Copilot as a reviewer is entirely opt-in. Initially, I will add it as a reviewer to test this. If the experiment is successful, we can explore how to integrate this into other parts of LLVM.