Update guidelines based on recent discussions within CQWG

bestpractices/c++practices.md

@@ -41,24 +41,22 @@ Raw loops are those starting with `for`, `while` etc. While there are many optio
 
 ```cpp
 // Pre-C++11:
-for(std::map<KeyType, ValueType>::iterator it = itemsMap.begin(); it != itemsMap.end(); ++it) {
-	//...
+for (std::map<KeyType, ValueType>::iterator it = itemsMap.begin(); it != itemsMap.end(); ++it) {
+    //...
     doSomething(*it);
-	//...
+    //...
 }
 
-// C++11
-for(auto item : itemsMap) {
-	//...
-	doSomething(item);
-	//...
+for (const auto& item : itemsMap) {
+    //...
+    doSomething(item);
+    //...
 }
 
-// C++17
-for(auto [name, value] : itemsMap) {
-	//...
-	doSomething(name, value);
-	//...
+for (const auto& [name, value] : itemsMap) {
+    //...
+    doSomething(name, value);
+    //...
 }
 ```
 
@@ -109,9 +107,9 @@ You can reduce six lines:
 int r; // can’t be const
 
 if (x == 2) {
-	r = 2;
+    r = 2;
 } else {
-	r = 3;
+    r = 3;
 }
 ```
 to one, with a single assignment, no curly braces, no repetition, and
@@ -124,13 +122,13 @@ This works especially great for simplifying return statements.
 If you have more than one ternary it might be worth to extract function that encapsulates the conditional logic:
 ```
 auto someConditionalLogic = []() {
-	if (condition1) {
-		return 1;
-	} else if (condition2) {
-		return 2;
-	} else {
-		return 3;
-	}
+    if (condition1) {
+        return 1;
+    } else if (condition2) {
+        return 2;
+    } else {
+        return 3;
+    }
 }
 
 const int r = someConditionalLogic();
@@ -305,44 +303,44 @@ for (const auto& [key, value] : map) { ... }
 > Time to rewrite.
 --- Linus Torvalds, 1995
 
-Indented code can be difficult to reason about, and fragile.
+Indented code can be difficult to reason about, and fragile.
 
 Main execution path should be the least indented one, i.e. conditions should cover specific cases. Early-Exit should be preferred to prune unwanted execution branches fast.
 
 Example:
 ```cpp
 if (something) {
-	doSomething();
-	if (somethingElse) {
-		doSomethingElse()
-		if (somethingElseAgain) {
-			doThing();
-		} else {
-			doDifferent();
-		}
-	} else {
-		doTheOther();
-	}
+    doSomething();
+    if (somethingElse) {
+        doSomethingElse()
+        if (somethingElseAgain) {
+            doThing();
+        } else {
+            doDifferent();
+        }
+    } else {
+        doTheOther();
+    }
 } else {
-	doNothing();
+    doNothing();
 }
 ```
 
 Can be changed into:
 ```cpp
 if (!something) {
-	doNothing();
-	return;
+    doNothing();
+    return;
 }
 doSomething();
 if (!somethingElse) {
-	doTheOther();
-	return;
+    doTheOther();
+    return;
 }
 doSomethingElse();
 if (!somethingElseAgain) {
-	doDifferent();
-	return;
+    doDifferent();
+    return;
 }
 doThing();
 ```
@@ -362,20 +360,20 @@ Using uninitialized POD type variables is undefined behavior.
 
 It _is_ OK to declare variables inside a loop.
 
-Prefer uniform initialization { } (also called brace initialization)(since C++11). Prevents narrowing.
+Prefer uniform initialization `{ }` (also called brace initialization), it prevents narrowing.
 
 Clarifies not assignment (=) or function call (()).
 
-Initialize class member variables at declaration, not in constructors (since C++11):
+Initialize class member variables at declaration, not in constructors:
 - Simplifies constructors
 - Avoids repetition
 - Establishes default state
 
 Static members can be defined in header file, no need to split to source file:
 ```cpp
 struct Something {
-	static const int _value_ = 2; // since C++98 for int
-	static inline std::string _something_ {"str"}; // since C++17
+    static const int _value_ = 2; // since C++98 for int
+    static inline std::string _something_ {"str"}; // since C++17
 }
 ```
 Lambdas can create and initialize variables in the capture, removing the
@@ -469,7 +467,7 @@ displayLines(80);
 
 Do the following instead:
 ```c++
-constexpr auto standardScreenLength {80};
+constexpr unsigned standardScreenLength {80};
 displayLines(standardScreenLength);
 ```
 
@@ -488,7 +486,7 @@ Use names that are specific. E.g. `SaveLogToDisk`, not `ProcessLog`. `Process` c
 A variable named after its data value defeats the whole point of a variable:
 ```cpp
 struct Dog {
-	std::string color {};
+    std::string color {};
 };
 Dog redDog {"blue"}; // BAD
 // 200 lines later, -_obviously_\- redDog is red! He’s blue? WTF?
@@ -508,7 +506,7 @@ Whether values are updated by the function is not obvious.
 Return value optimizations (RVO) simplifies return and usually elides copies.
 ```cpp
 auto func = [](const auto& str, const auto num) {
-	return { str, num };
+    return { str, num };
 };
 ```
 Structured binding (since C++17) simplifies reading back the result at the calling side:
@@ -578,7 +576,7 @@ Item martha { "Martha", 30 };
 Item george { "George", 40 };
 ```
 
-Move the data into a container e.g. `constexpr std::array`, not a vector or map.
+Move the data into a container e.g. `constexpr std::initializer_list`, not a vector or map.
 
 Container elements are typically value, array, pair, tuple, or a defined struct.
 Pair and tuple should only be used for very short lived data, such as this case:
@@ -587,32 +585,28 @@ Pair and tuple should only be used for very short lived data, such as this case:
 ```cpp
 using Pair = std::pair<std::string_view, size_t>;
 
-constexpr auto numItems {3};
-
-constexpr std::array<Pair, numItems> items {{
-	{ "Fred", 20 },
-	{ "Martha", 30 },
-	{ "George", 40 }
-}};
+constexpr std::initializer_list<Pair> items {
+    { "Fred", 20 },
+    { "Martha", 30 },
+    { "George", 40 }
+};
 ```
 {% endraw %}
 
 A struct can also be used, which has the advantage of named elements, but is slightly more overhead.
 {% raw %}
 ```cpp
 struct Button {
-	std::string_view name;
-	size_t height;
-	size_t width;
+    std::string_view name;
+    size_t height;
+    size_t width;
 };
 
-constexpr auto numButtons {3};
-
-constexpr std::array<Button, numButtons> buttonDefs {{
-	{ "Go", 25, 25 },
-	{ "Get set", 20, 20 },
-	{ "On your marks", 15, 15 }
-}};
+constexpr std::initializer_list<Button> buttonDefs {
+    { "Go", 25, 25 },
+    { "Get set", 20, 20 },
+    { "On your marks", 15, 15 }
+};
 ```
 {% endraw %}
 When in doubt, use a struct - it is better to have good names than not.

bestpractices/codereview.md

@@ -17,7 +17,7 @@ While points of this guide might not be consistently followed throughout all of
 In this document the **bolded** text will indicate how important each suggestion is.
  - **must** will be used for fundamental things that should be non-controversial and which you really should follow
  - **should** will be used for important details that will apply for vast majority of cases, there could however be valid reasons to ignore them depending on context
- - **could** will be used for best practices, things that you should try to follow but not following them is not an error per se
+ - **could**/**can** will be used for best practices, things that you should try to follow but not following them is not an error per se
 
 ## Common Rules
 1. Consistency **must** be preferred over strict rule following. If, for example, in a given context code uses different naming scheme - it should be followed instead of one described in that document.
@@ -32,6 +32,22 @@ In this document the **bolded** text will indicate how important each suggestion
 10. New code **must not** introduce any new linter warnings
 11. Developer **should** have `pre-commit` installed and working. `pre-commit-ci` commits should be avoided.
 
+## PRs and Commits
+1. Commit message **should** include prefix with module name, e.g. `Part: `
+2. Commit message **should** use imperative mode - just like git does, e.g. `Part: Add fillet feature`
+    <details>
+    A properly formed Git commit subject line should always be able to complete the following sentence:
+
+    If applied, this commit will **your commit message**,
+
+    e.g. If applied, this commit will add fillet feature.
+    </details>
+3. If changeset is big and spans across multiple modules it **should** be split into one commit per module.
+4. Every commit in the PR **must** compile and run properly. `git bisect` workflow does not work if some commits are not possible to compile.
+5. Commit message **can** contain additional information after one blank line.
+6. Every PR **should** have description, even if it seems obvious.
+
+
 ## Basic Code Rules
 1. (C++ only) New code **must** be formatted with clang-format tool or in a way that is compatible with clang-format result if file is excluded from auto formatting.
 1. (Python only) Code **must** follow the PEP 8 standard and formatted with Black.
@@ -79,17 +95,37 @@ In this document the **bolded** text will indicate how important each suggestion
 11. (C++ only) Return values **should** be preferred over out arguments.
     a. For methods that can fail `std::optional` should be used
     b. For methods that return multiple values it may be better to either provide dedicated struct for result or use `std::tuple` with expression binding.
-12. If expression is not obvious - it **should** be given a name by using variable.
+13. (C++ only) If dealing with pointers `auto` **must** be suffixed with `*`: `auto*`.
+14. (C++ only) If dealing with references `auto` **must** be suffixed with `&`: `auto&`
+15. (C++ only) `auto` **should** be used if:
+    a. the right-hand side makes clear which type it is, e.g. with `static_cast`, or `new`
+    b. the type is long or verbose (iterators, ranges, ...)
+    c. it reduces redundancy in code (repeated type)
+15. (C++ only) `auto` **should not** be used if:
+    a. dealing with primitives (the type is not always clear, int vs unsigned etc)
+    b. the type is not clear from context, method name or template arguments
+16. If expression is not obvious - it **should** be given a name by using variable.
     <details>
         <summary>Example #1</summary>
         TODO: Find some good example
     </details>
-13. (C++ only) Code **should not** be put in anonymous namespace.
-14. All members **must** be initialized.
+17. (C++ only) Code **should not** be put in anonymous namespace.
+18. All members **must** be initialized.
     <details>
         <summary>Rationale</summary>
         Not initialized members can easily cause undefined behaviors that are really hard to find.
     </details>
+19. If possible, functions / methods **should** not use more than 3 indentation levels.
+    <details>
+        <summary>Rationale</summary>
+        Main path of code should be the least indented, exceptional cases should be handled as
+        early-exit if statements. Code that is highly indented is harder to process and often
+        has large cognitive load - if something is not possible to write within 3 indentation
+        levels it is a good signal that it might require splitting into smaller methods.
+    </details>
+20. (C++ only) dedicated casts should be preferred over `dynamic_cast` due to performance
+    a. QObjects should use `qobject_cast`
+    b. FreeCAD types (deriving from `BaseClass`) should use `freecad_cast`
 
 ## Design / Architecture
 1. Each class / method / function **should** be doing only one thing and should do it well.
@@ -194,13 +230,29 @@ In this document the **bolded** text will indicate how important each suggestion
 
 
 ## Naming Things
+
+Naming described in that section of the document refers mostly to newer parts of code. Some modules use different naming conventions,
+while checking the code Reviewer should take into account surrounding code and first of all ensure consistency within the closes scope.
+
 1. Code symbols (classes, structs, methods, functions, variables...) **must** have names that are meaningful and grammatically correct.
 2. Variables **should not** be named using abbreviations and/or 1 letter names. Iterator variables or math related ones like `i` or `u` are obviously not covered by this rule.
 3. Names **must not** use the hungarian notation.
-4. (C++ only) Classes/Structs **must** be written in `PascalCase`, underscores are allowed but should be avoided.
-5. (C++ only) Class members **should** be written in `camelCase`, underscores are allowed but should be avoided.
+4. (C++ only) Classes/Structs/Enums **must** be written in `PascalCase`, underscores are allowed but should be avoided.
+5. (C++ only) Class/Struct members **should** be written in `camelCase`, underscores are allowed but should be avoided.
 6. (C++ only) Global functions **should** be written in `camelCase`, underscores are allowed but should be avoided.
 7. (C++ only) Enum cases **should** use `PascalCase`
+9. (C++ only) Enums should use singular form (i.e. `enum ObjectState` instead of `enum ObjectStates`)
+8. (C++ only) Local variables **should** use `camelCase`
+
+### Naming Conventions
+1. Coin nodes **should** be prefixed with `So`, e.g. `SoTransformDragger`. Reimplementation of existing Coin nodes **should** use `SoFC` prefix, e.g. `SoFCTransform`.
+2. Features in Part and Part Design workbenches **should** be prefixed with `Feature`, e.g. `FeatureHole`.
+3. View Providers corresponding to features / other objects **should** be prefixed `ViewProvider`, e.g. `ViewProviderHole`
+4. Task Views (content for task panels) **should** be prefixed with `Task`, e.g. `TaskTransform`
+5. Task Dialog for Task View **should** be additionally suffixed with `Dialog`, e.g. `TaskTransformDialog`
+6. Preference Pages **should** be prefixed with `DlgSettings`, e.g. `DlgSettingsLights`.
+7. Commands **should** be prefixed with `Cmd` and Workbench name e.g. `CmdPartDesignCreateSketch`
+
 
 ## Commenting the code
 1. Good naming **must** be preferred over commenting the code.