# How to make the perfect pull request
###### tags: `admin` `git` `coding`
Some of the rules below are checked via `revng check-conventions`. However care must be taken as not all of them are checked automatically. For now, the rest it's up to you!
The better your PRs, the more confidence the maintainers will have in you and the faster the PR will go through!
## A. `git`/PRs
1. **Rebase** over `develop` after each review
2. Commit message
1. Respect the **50/72 rule**:
* First line max 50 chars
* One blank line
* As much text as you wish, wrapped at 72 chars
2. First letter uppercase, no trailing dot
3. The commit author must be `Name Surname <user@rev.ng>`
Do not use your personal e-mail!
4. Do not sign commits
5. Describe what you did in the patch using the *imperative mood*.
Imagine you're telling yourself what to do, e.g., "Implement the deduplication logic".
3. Do **one thing per commit**.
1. If you move a file or a significant amount of code, have a dedicated commit.
2. If you need to change conventions (e.g., indentation), do it in a dedicated commit.
4. **No corrections**: squash together commits that fix errors introduced in a previous commit in the same branch.
**NO**:
```diff
$ git log -p
commit a78e0d266ddd47145b3736f5b4bf2e84bbe8c109 (HEAD -> master)
Fix mistake
diff --git a/NEWFILE b/NEWFILE
index 90d51b6..b00d637 100644
--- a/NEWFILE
+++ b/NEWFILE
@@ -1 +1 @@
-Your great
+You're great
commit 8248a389d361648a9fcbda3a687c3b04b5093975
Introduce NEWFILE
diff --git a/NEWFILE b/NEWFILE
new file mode 100644
index 0000000..90d51b6
--- /dev/null
+++ b/NEWFILE
@@ -0,0 +1 @@
+Your great
```
5. <a id="small-fixes-first"></a>**Small fixes first**: put first commits that are small fixes that are beneficial for the project, even if the current PR is not merged immediately.
They might get merged earlier.
6. **Don't use `master`/`develop` as source branch** for a PR.
Make a branch with an appropriate name starting with `feature/`.
`feature/` is the only allowed prefix. Do not make up other prefixes such as `fix/`.
7. If one or more commits are shared among PRs (e.g., common fixes):
* They should be the first commits in all the PR that include them.
* The description of the PR shold state what commits are shared with other PRs:
```
First $n commits are shared with (#|!)<issue id>
```
8. When a change spans multiple repositories, add links to the "sister" PRs in the description of the PR in the main repo. For example:
```
Sister PRs:
* [orchestra](https://github.com/revng/orchestra/pull/###)
* [revng](https://github.com/revng/revng/pull/###)
... etc.
```
9. If a change spans multiple repositories, make sure that the branch name used in all of them is the same
```
# Not OK
revng PR: merge `feature/add-foo` to `master`
orchestra PR: merge `feature/add-foo-in-revng` to `master`
# OK
revng PR: merge `feature/add-foo` to `master`
orchestra PR: merge `feature/add-foo` to `master`
```
10. After a PR review round, **comments that can be quickly answered should be addressed as soon as possible**, as this reduces the latency for feedback.
11. **Do not link diffs of an open PR** (e.g., `https://github.com/revng/revng/pull/<ID>/commits/<XXX>#diff-<YYY>R<NN>`), these become invalid as soon as another commit is pushed, making them useless to refer back to. Instead either:
* Describe in the comment what aspects of the changed code are important
* If the change is small, paste it directly in the comment
12. **If you're reasonably sure you addressed a comment, mark it as resolved**.
In case of significant deviations discuss the issue at hand with the reviewer before implementing a fix.
13. **Do not mark a comment as resolved until you push the fix**.
If you want to mark something as done for yourself, add a :+1: reaction to the comment.
14. **Run `revng check-conventions --force-format`** frequently.
This avoids having to deal with formatting changes all at once at the end of development.
15. For projects under the rev.ng umbrella, **run `orc check-branch`** before submitting a PR.
This ensures that:
1. each commit respects the project's conventions and that
2. each commit builds successfully.
From an orchestra directory:
```bash
orc check-branch <component-name>
# If something goes wrong
cd sources/<component-name>
# 1. Check the changes
git diff
# 2. If they look good apply them!
git commit -a --amend --no-edit
# 3. Proceed
git rebase --continue
```
16. Ensure the project's **test suite passes** successfully on the last commit of the branch.
The rule is: `revng check-conventions` must pass on each commit, tests need to pass on the last commit of the branch.
## B. Language-agnostic practices
1. **Do not abbreviate variable names**: they are ambigous and they make the code hard to read.
Some exceptions apply when the variable belongs to a small context, for example:
```python
# Python, using 'v' in comprehension is OK
mapped_values = [foo(v) for v in values]
```
```typescript
// Typescript, using 'e' when in map/filter
good_values = values.filter((e) => e.isGood);
```
2. Comments:
* **start with a capital letter**;
* **always use `//`** instead of `/* ... */`;
* **always put comments on their own line**;
```diff
- my_function(); // Call my_function
+ // Call my_function
+ my_function();
```
* **must not end with a full stop `.`**, unless the comment is composed by multiple sentences.
```diff
- // Call my_function.
+ // Call my_function
my_function();
```
3. **No magic numbers**: depending on the situation the choices should be:
* `enum`s in the case of many-valued variables:
```cpp
enum Directions {
North,
South,
West,
East
};
```
* Constants when there's a single value:
```cpp
const std::string ElfMagic = "\x{7F}ELF";
```
4. **Do not declare more than one variable per line**. Only exception is for tuple unpacking:
```c++
// C++
auto [Key, Value] = *Map.find(Key);
```
```python
// Python
foo, bar = doThing()
```
```typescript
// Typescript
const [foo, bar] = doThing()
```
5. **Write messages for users on `stderr`**, even if they are not an error.
Use `stdout` only if you expect another program to consume it.
`stderr` is for "informal" output, `stdout` is for "structured" output.
6. **Be careful when creating temporary files**.
1. Always provide a sensible prefix.
```bash
# Bash
# Not OK
TEMP_FILE="$(mktemp)"
# OK
TEMP_FILE="$(mktemp --tmpdir tmp.example-script.XXXXXXXXXX)"
```
```python
# Python
# Deleted once "file" is not referenced anymore
file = NamedTemporaryFile(prefix="tmp.example.")
# Explicit, will be deleted once out of the "with"
with NamedTemporaryFile(prefix="tmp.example.") as f:
...
```
```cpp
// C++
TemporaryFile File("tmp.example.");
```
2. Use "managed" facilities for temporary files which automatically clean up those files on exit.
* Python: [`TemporaryFile`](https://docs.python.org/3/library/tempfile.html#tempfile.TemporaryFile), [`NamedTemporaryFile`](https://docs.python.org/3/library/tempfile.html#tempfile.NamedTemporaryFile), [`TemporaryDirectory`](https://docs.python.org/3/library/tempfile.html#tempfile.TemporaryDirectory) (all in the [`tempfile`](https://docs.python.org/3/library/tempfile.html) module)
* C++: `TemporaryFile` (in `revng/Support/TemporaryFile.h`), `llvm::sys::fs::createTemporaryFile`, `llvm::sys::fs::createUniqueDirectory`
* Bash: there isn't a facility built-in, do:
```bash
# Single file
TEMP_FILE=$(mktemp --tmpdir tmp.example.XXXXXXXXXX)
trap 'rm -f "$TEMP_FILE"' EXIT
# More complex
TEMP_FILE_1=$(mktemp ...)
TEMP_FILE_2=$(mktemp ...)
function cleanup() {
rm -f "$TEMP_FILE_1"
rm -f "$TEMP_FILE_2"
}
trap cleanup EXIT
```
7. **Never use `revng` or `Revng` as a name prefix**.
In C++ you can use the `revng` namespace.
In Python you can use the `revng` module.
In the file system you can use a `revng` directory (e.g., `$HOME/.local/revng`).
```diff
- revng_fooinator = RevngFooinator()
+ fooinator = Fooinator()
```
8. **Document temporary hacks thoroughly and with conditions that allow their removal**.
```diff
- int foo = doTheFooHack();
+ // Obtain the foo value via doTheFooHack, this is needed because libFoo
+ // does not yet provide the foo value directly. When PR#1234 of libFoo is
+ // merged the foo value can be obtained directly via getFoo() and the hack
+ // can be removed.
+ int foo = dooTheFooHack();
```
## C. C/C++
1. Try to **modernize the code base**, or at least stop using banned practices.
These include:
1. No format strings (`printf`), use [`llvm::Twine`](https://llvm.org/doxygen/classllvm_1_1Twine.html) or [`llvm::raw_ostream`](https://llvm.org/doxygen/classllvm_1_1raw__ostream.html).
2. No C strings (`strlen`, `strcpy`, ...), use [`llvm::StringRef`](https://llvm.org/doxygen/classllvm_1_1StringRef.html) if you don't need to change the string or [`llvm::SmallString`](https://llvm.org/doxygen/classllvm_1_1SmallString.html) otherwise.
3. Do not introduce **unnecessary dependencies** (e.g., Boost), unless you're sure LLVM does not provide a valid alternative.
4. **Use [`llvm::Error`](https://llvm.org/doxygen/classllvm_1_1Error.html) or [`llvm::Expected<T>`](https://llvm.org/doxygen/classllvm_1_1Expected.html)** for error handling.
Do not use exceptions.
Do not use [`llvm::ErrorOr`](https://llvm.org/doxygen/classllvm_1_1ErrorOr.html).
Do not use [`std::error_code`](https://en.cppreference.com/w/cpp/error/error_code).
If any API forces you to use one of the above, convert them to `llvm::Error` right away.
The following functions are banned since they throw exceptions in case of error: `std::stoll`.
5. **Always check functions returning an `llvm::Error` or `llvm::Expected<T>`**.
Suppose you have a function returning a `llvm:Error` or a `llvm::Expected<T>`:
```cpp
llvm::Error migthFailWithError();
llvm::Expected<int> migthFailWithExpected();
```
You need to either:
1. propagate it up by returning it:
```cpp
llvm::Expected<int> forwardOnError() {
if (auto Error = migthFailWithError())
return Error;
return 3;
}
llvm::Expected<int> forwardOnError2() {
auto MaybeInt = migthFailWithExpected();
if (not MaybeInt)
return MaybeInt.takeError();
return *MaybeInt;
}
```
2. print its message and `abort`:
```cpp
int abortOnError() {
cantFail(migthFailWithError());
return 3;
}
int abortOnError2() {
return cantFail(migthFailWithExpected());
}
```
3. if it's non-fatal, print it to a `Logger`:
```cpp
int logOnError() {
if (auto Error = migthFailWithError()) {
// The call to toString *must not* be in revng_log!
std::string Message = llvm::toString(std::move(Error));
revng_log(Log, "Failure: " << Message);
}
return 3;
}
int logOnError2() {
auto MaybeInt = migthFailWithExpected();
if (not MaybeInt) {
// The call to toString *must not* be in revng_log!
// revng_log(Log, ...) is equivalent to if (Log.isEnabled()) { ... }
std::string Message = llvm::toString(MaybeInt.takeError());
revng_log(Log, "Failure: " << Message);
return 3;
}
return *MaybeInt;
}
```
**`revng_log` second argument is evaluated only if the logger is enabled! If you call `toString` in `revng_log` and the logger is disabled, you will not "consume" the error and there will be a crash!**.
4. suppress the error silently; do not dot this.
```cpp
int suppressOnError() {
if (auto Error = migthFailWithError()) {
llvm::consumeError(std::move(Error));
}
return 3;
}
int suppressOnError2() {
auto MaybeInt = migthFailWithExpected();
if (not MaybeInt) {
llvm::consumeError(MaybeInt.takeError());
return 3;
}
return *MaybeInt;
}
```
6. **Mutexes must not be scattered around the codebase**. They should be concentrated in a few "infrastructure" points that are obvious to reason about and offer strong and clear guarantees to their users.
7. **Do not use `std::cerr` or `llvm::dbgs()`**, use `dbg`.
8. Functions should never accept a `const std::vector<T> &` or `const std::string &`: **use `llvm::ArrayRef<T>` or `llvm::StringRef`**.
9. **Do not abuse `auto`**. Use `auto` only if:
1. The line is very long and you can't figure any other way to make `check-conventions` happy.
2. The type is obvious since it's explicitly stated on the same line (e.g., `auto *Lol = dyn_cast<CallInst>(...)`).
3. The type is very ugly (typically, iterators).
10. **Organize your classes** as follows (don't put the comments):
```cpp
class MyClass {
private:
// Private usings, if any
using MyPrivateUsing = int;
public:
// public usings, if any
using MyPublicUsing = int;
private: // Or public
// Fields of the class
MyPublicUsing X;
public:
// Constructors
MyClass() = default;
MyClass(int X) : X(X) {}
public:
// Static factory methods
static MyClass fromFloat(float X);
public:
// Accessor methods
MyPublicUsing x() const { return X; }
public:
// Non-accessor `const` methods
int twice() const { return multiply(2); }
public:
// Non-accessor const methods
void clear() { X = 0; }
private:
// Helper private methods
int multiply(int Op) const { return X * Op; }
};
```
11. If your class is supposed to be used by others, **add a `dump` and `verify` methods**:
```cpp
class MyClass {
// ...
public:
void dump() const debug_function {
dump(dbg);
}
template<typename T>
void dump(T &Stream) {
// ...
}
bool verify() const debug_function {
return ...;
}
}
```
12. Classes should **follow the [rule of 3/5/0](https://en.cppreference.com/w/cpp/language/rule_of_three)**.
13. **Cleanly split private and public interface**: each library `librevngMyLib.so` has a directory for the public interface (`include/revng/MyLib`).
Limit what you put there to what's interesting to your users, keep all the rest in `lib/MyLib/*.cpp` or create a private header in `lib/MyLib/*.h` if a `class` is used by multiple `.cpp` files within the library.
14. Always initialize class members that have a primitive type.
```diff
struct MyStruct {
- bool Boolean;
- int Integer;
- int *Pointer;
+ bool Boolean = false;
+ int Integer = 0;
+ int *Pointer = nullptr;
// Non-primitive
std::string String;
}
```
15. **Prefer [alternative operator representations](https://en.cppreference.com/w/cpp/language/operator_alternative) for `and`, `or` and `not`**.
```diff
-if (!condition)
+if (not condition)
```
16. No trailing commas in `enum`s.
17. Add braces to if/else/for if the body is longer than 1 line
```cpp
// OK
if (var)
doThing();
// OK
if (var) {
doThing();
doOtherThing();
}
// OK
if (auto var = getFromSomewhere())
for (auto x : var)
doThing(x);
// Not OK
if (var)
doThing(longVariableName1,
longVariableName2);
// Not OK
if (var)
// My comment
doThing(longVariableName1);
// OK
if (var) {
doThing(longVariableName1,
longVariableName2);
}
// OK
if (var)
doThing();
else
doOtherThing();
// Not OK (if either `if` or `else` clause needs brace - both should have them)
if (var)
doThing();
else {
doThing(longVariableName1,
longVariableName2);
}
```
18. Avoid using `const` for:
* function parameters that are implicitly const (e.g. `llvm::StringRef`, `llvm::ArrayRef`);
* values that will be returned by copy from a function;
* variables within the body of a function.
19. When using lambdas, don't capture `&`, but spell explicitly all the necessary captures.
```c++
// Not OK
[&]() { return A + B; }
// OK
[&A, &B]() { return A + B; }
```
## D. Python
1. When in doubt, consult [`PEP-0008`](https://peps.python.org/pep-0008/).
2. **Do not use `print()` for logging**.
Either:
* in stand-alone files use
```python
def log(message):
sys.stderr.write(message + "\n")
sys.stderr.flush()
```
* if within a framework, use that the framework suggests (e.g., `logging` when in Django).
3. **Always use type annotations**.
4. For functions, use the most generic type possible for arguments and the exact type for return values.
```python
# Not OK, bar and baz have too strict type
def foo(bar: List[str], baz: Dict[str, str]) -> List[str]:
...
# Not OK, return is too generic
def foo(bar: Iterable[str], baz: Mapping[str, str]) -> Iterable[str]:
...
# OK
def foo(bar: Iterable[str], baz: Mapping[str, str]) -> List[str]:
...
```
5. **Do not use names starting with `__`**.
6. **Do not re-use the same variable name for different types**.
```python
# Not OK
data = read_data() # type is 'Dict[str, str]'
data = process_data(data) # type is 'Config'
# OK
data = read_data() # type is 'Dict[str, str]'
data = process_data(data) # type is 'Dict[str, str]'
config = make_config(data) # type is 'Config'
```
7. **All imports should be at the beginning of the file**. Special exceptions can be made if the import is particularly expensive or if it's an optional dependency, e.g.:
```python
if should_do_operation_foo:
import expensive_library
expensive_library.operation_foo()
```
8. **Use `dataclass`es abundantly**.
```python
# Not OK
def foo() -> Dict[str, str | int]:
return {
"foo": "foo",
"bar": 2
}
# OK
@dataclass
class FooData:
foo: str
bar: int
def foo() -> FooData:
return FooData("foo", 2)
```
## E. TypeScript/JavaScript
1. When handling `package.json` dependency changes:
* if you're introducing new dependency, keep the `package.json` and `package-lock.json` changes in the same commit;
* if you're bumping dependencies (e.g., using `npm upgrade`), keep the changes at the beginning of the branch as a separate commit;
2. When a callback requires `this` to be bound, prefer the use of an arrow function to `thisArg`:
```typescript
// Not OK
arr.filter(function (e) { return this.check(e); }, this);
// OK
arr.filter((e) => this.check(e));
```
3. Avoid using `const` to define functions (unless `this` needs to be captured)
```typescript
// OK
function foo() {
doTheFoo();
}
// Not OK
const foo = () => {
doTheFoo();
}
```
## F. Bash
1. **Always start scripts with**:
```bash
#!/usr/bin/env bash
set -euo pipefail
<rest of the code>
```
2. All variable names should be `UPPERCASE`
```bash
FOO="foo" # OK
Foo="foo" # Not OK
for FOO in "${FOOS[@]}"; do
do_foo "$foo"
done
process_files *.txt | \
while IFS= read -r LINE; do
process_line "$line"
done
```
3. **Always run `shellcheck`**.
Specifically, use the following rules:
* surround variables with `"`
* use `:-` for possibly missing variables, e.g. `${VAR:-}`
* use `IFS= read -r` to read a file line-by-line
4. Follow "egyptian style" for conditions/loop
```bash
# Not OK
if [ -z "$1" ]
then
do_thing
fi
while true
do
do_thing
done
# OK
if [ -z "$1" ]; then
do_thing
fi
while true; do
do_thing
done
```
5. Avoid using `which` as it might not be installed in all distros, prefer `command -v` instead
```bash
# Not OK
PROG_PATH="$(which prog)"
if which prog &> /dev/null; then
prog "$PROG_ARGS"
fi
# OK
PROG_PATH="$(command -v prog)"
if command -v prog &> /dev/null; then
prog "$PROG_ARGS"
fi
```
6. Prefer the `&>` and `|&` notation rather than `>&` and `2>&1`
```bash
# Not OK
loud_program > /dev/null 2>&1
loud_program >& /dev/null
other_program 2>&1 | grep "foo"
# OK
loud_program &> /dev/null
other_program |& grep "foo"
```