|
| 1 | +# Instructions for Code Reviews |
| 2 | + |
| 3 | +You will analyze the files which have been added and/or changed in this pull request. You will consider correctness, |
| 4 | +best practice, and the Coding Standards provided below. Where there are |
| 5 | +conflicts between best practice and the Coding Standards, you will use the Coding Standards. |
| 6 | + |
| 7 | +These standards represent guidelines. Deviations from them are permitted, if there is a reasonable |
| 8 | +justification provided in comments. Such deviations should only be permitted if it makes the code |
| 9 | +easier to understand. |
| 10 | + |
| 11 | +You will diligently analyze each of the files which are touched by the pull request. You will review the |
| 12 | +changes (diff hunks) submitted in the pull request, considering their correctness, best practice, and adherence to the Coding Standards. |
| 13 | + |
| 14 | +You will consider every bullet point in the Coding Standards. You will note all discrepancies in your review. |
| 15 | +You will provide the minimal set of changes which will bring the code into compliance with best practice and |
| 16 | +the Coding Standards. Your proposed changes should conform to best practice and the Coding Standards. |
| 17 | + |
| 18 | +For each change you propose, you should explain why you are proposing that change. Be concise in your response. |
| 19 | + |
| 20 | +You will create a table summarizing the code review. For each file touched in the pull request, explicitly state the nature |
| 21 | +of the changes which were made, whether they conform to best practice, and whether they conform to the Coding |
| 22 | +Standards enumerated below. Where they fail the coding standards, indicate each of the standards which were violated. |
| 23 | + |
| 24 | +Recommended changes should always follow the Coding Standards. |
| 25 | + |
| 26 | +# Coding Standards |
| 27 | + |
| 28 | +Applicable languages: c++11, C99, FORTRAN 77, python 2.7, perl 5, XML. |
| 29 | + |
| 30 | +Where guidance applies to a specific language, it will be noted in square brackets. e.g. [c++]. |
| 31 | + |
| 32 | +Follow the principle of least surprise. Software and interfaces should behave in a way that is most predictable and intuitive for developers |
| 33 | +and users, minimizing unexpected or surprising actions. |
| 34 | + |
| 35 | +Formatting style not mandated except for fixed-format (FORTRAN77, python). |
| 36 | + |
| 37 | +Follow best practices unless overridden below. |
| 38 | + |
| 39 | +Unit tests may deviate from these conventions. |
| 40 | + |
| 41 | +## File/Class Naming [c++] |
| 42 | +* `.h` for headers, `.cxx` for implementations. |
| 43 | +* One class/struct or related group per header. |
| 44 | +* Classes start with `St`, prefer CamelCase (legacy snake_case allowed). |
| 45 | + |
| 46 | +## Headers [c++] |
| 47 | +* Each implementation has a header. |
| 48 | +* Use include guards: `__FILEBASENAME_H__`. |
| 49 | +* Define functions in implementation files, except short `inline` after class. |
| 50 | +* Don't inline virtuals. |
| 51 | +* Use angle brackets for external, quotes for project headers. |
| 52 | +* Use only necessary includes; minimize dependencies with forward declarations where appropriate. |
| 53 | + |
| 54 | +## Namespaces [c++] |
| 55 | +* Avoid namespaces in STAR; use STAR file/class naming conventions to prevent collisions. |
| 56 | + |
| 57 | +## Scoping [general] |
| 58 | +* Declare variables in narrowest scope possible. |
| 59 | +* Init all variables. |
| 60 | +* Prefer brace initialization (except single-argument assignment) [c++]. |
| 61 | +* Never brace-initialize with `auto` [c++]. |
| 62 | +* No global vars; static class/namespace variables are discouraged. [c++] |
| 63 | +* If global variables are necessary, initialize them statically. |
| 64 | + |
| 65 | + |
| 66 | +## Classes [c++] |
| 67 | +* Every class must declare at least one ctor, even if defaulted with `= default;`. |
| 68 | +* Every class must declare a ctor which can be called with no parameters. A ctor with all default parameters is allowed. |
| 69 | +* Classes which allocate memory and retain ownership should deallocate memory in the dtor. |
| 70 | +* Initialize all data members. |
| 71 | +* Don't call virtuals in ctors/dtors. |
| 72 | +* Implement/delete assignment/copy ctors (compiler defaults OK if no heap). |
| 73 | +* Use delegating/inheriting ctors to avoid duplication. |
| 74 | +* Use `struct` only for plain-old-data (POD) types. |
| 75 | +* Prefer composition over inheritance. |
| 76 | +* Classes which use inheritance should only use public inheritance |
| 77 | +* Only one concrete base; others must be pure interfaces (no concrete methods). |
| 78 | +* Mark overrides as `override` or `final`. |
| 79 | +* Follow the principle of least surprise: preserve operator semantics. |
| 80 | +* Explicitly use public/protected/private; list public first. |
| 81 | +* Hide internals; don't return handles to internal data. |
| 82 | +* Avoid `friend`. |
| 83 | + |
| 84 | +## ROOT [c++] |
| 85 | +* Prefer C++ types over ROOT types, except persistent classes. |
| 86 | + * e.g. prefer int, float, double, char, bool over Int_t, Float_t, Double_t, Char_t, Bool_t. |
| 87 | +* Prefer `<cmath>` over ROOT math (TMath::*). |
| 88 | + |
| 89 | +## Introspection [c++] |
| 90 | +* Prefer `auto` over `decltype`. |
| 91 | +* Avoid macros. |
| 92 | +* Avoid dynamic_cast except for error detection. |
| 93 | + |
| 94 | +## General |
| 95 | +* Avoid magic numbers. |
| 96 | +* Keep functions small/focused. |
| 97 | +* Avoid exceptions/asserts unless necessary [c++]. |
| 98 | + |
| 99 | +## Miscellaneous [c++] |
| 100 | +* Prefer strong enums. |
| 101 | +* Avoid lambdas for frequent calls or reference capture. |
| 102 | +* Use `const` for logically constant objects; design const-correct interfaces. |
| 103 | +* Use `constexpr` for true constants or constant init. |
| 104 | +* Avoid smart pointers (ROOT ownership). |
| 105 | +* Avoid casting; use `static_cast` if needed, avoid `const_cast`, `reinterpret_cast`, and C-casts. |
| 106 | +* No variable-length arrays or `alloca()`. |
| 107 | +* Prefer prefix ++/--. |
| 108 | +* Switch: cover all cases, use default if needed. |
| 109 | +* No empty loop bodies; use `continue` or empty block. |
| 110 | +* Declare iteration vars in loop conditions. |
| 111 | +* Use range-for and (const) reference where possible. |
| 112 | +* Prefer iterator if index needed. |
| 113 | +* Use `int` unless fixed size needed (`<cstdint>`). |
| 114 | +* Ensure portability (printing, comparison, alignment). |
| 115 | +* Use `nullptr` for pointers. |
| 116 | +* Prefer `sizeof(var)` to `sizeof(type)`. |
| 117 | +* Use `auto` for local vars if it improves readability. |
| 118 | +* Use non-member `begin()`/`end()`. |
| 119 | + |
| 120 | +## Non-Conformant Code |
| 121 | +* Deviations allowed for legacy code. |
| 122 | +* Refactor incrementally, one deviation at a time, preserving behavior. |
| 123 | + |
| 124 | + |
| 125 | + |
| 126 | + |
| 127 | + |
0 commit comments