Skip to content

Commit 58062e4

Browse files
authored
[contributing] Update C++ field visibility guidance to align with codebase practice (#64)
1 parent fa0e6ee commit 58062e4

File tree

1 file changed

+72
-2
lines changed

1 file changed

+72
-2
lines changed

docs/contributing/code.md

Lines changed: 72 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -16,16 +16,86 @@ We use the [Google C++ Style Guide](https://google.github.io/styleguide/cppguide
1616
- Function, method and variable names are `lower_snake_case`
1717
- Class/struct/enum names should be `UpperCamelCase`
1818
- Constants should be `UPPER_SNAKE_CASE`
19-
- Fields should be `protected` and `lower_snake_case_with_trailing_underscore_` (DO NOT use `private`)
19+
- Fields should be `lower_snake_case_with_trailing_underscore_` and:
20+
- **Prefer `protected`** for most fields to allow extensibility and testing
21+
- **Use `private`** for true implementation details, especially when direct access could lead to bugs:
22+
- **Pointer lifetime issues**: When a setter validates and stores a safe pointer from a known list (e.g., storing
23+
`current_option_` pointer that must point to an entry in `options_` vector, not a temporary string)
24+
- **Invariant coupling**: When multiple fields must stay synchronized (e.g., `data_` and `size_` must always match)
25+
- **Resource management**: When a setter performs cleanup/registration (e.g., unregistering old sensor before
26+
storing new one)
27+
- Provide `protected` accessor methods when derived classes need controlled access to `private` members
2028
- It's preferred to use long variable/function names over short and non-descriptive ones.
2129
- All uses of class members and member functions should be prefixed with `this->` to distinguish them from global
2230
functions/variables.
2331
- Use two spaces, not tabs.
24-
- Using `#define` is discouraged and should be replaced with constants or enums (if appropriate).
32+
- Using `#define` for constants is discouraged and should be replaced with `const` variables or enums. Use `#define` only for:
33+
- Conditional compilation (`#ifdef`, `#ifndef`)
34+
- Compile-time sizes calculated during Python code generation (e.g., `cg.add_define("MAX_SERVICES", count)` for `std::array` sizing)
2535
- Use `using type_t = int;` instead of `typedef int type_t;`
2636
- Wrap lines in all files at no more than 120 characters. This makes reviewing PRs faster and easier. Exceptions
2737
should be made only for lines where wrapping them would result in a syntax issue.
2838

39+
#### When to use `private` vs `protected`
40+
41+
##### Example: Pointer lifetime safety
42+
43+
```cpp
44+
class ClimateDevice : public Component {
45+
public:
46+
void set_custom_fan_modes(std::initializer_list<const char *> modes) {
47+
this->custom_fan_modes_ = modes;
48+
this->active_custom_fan_mode_ = nullptr; // Reset when modes change
49+
}
50+
51+
bool set_custom_fan_mode(const char *mode) {
52+
// Find mode in supported list and store that pointer (not the input pointer)
53+
for (const char *valid_mode : this->custom_fan_modes_) {
54+
if (strcmp(valid_mode, mode) == 0) {
55+
this->active_custom_fan_mode_ = valid_mode;
56+
return true;
57+
}
58+
}
59+
return false; // Mode not in supported list
60+
}
61+
62+
protected:
63+
// Protected: Simple state that derived classes can safely access
64+
bool has_state_{false};
65+
66+
private:
67+
// Private: Pointer that MUST point to entry in custom_fan_modes_ vector
68+
std::vector<const char *> custom_fan_modes_; // Pointers to string literals in flash
69+
const char *active_custom_fan_mode_{nullptr};
70+
};
71+
72+
// If active_custom_fan_mode_ was protected, a derived class could do:
73+
// this->active_custom_fan_mode_ = some_temporary_string; // Use-after-free bug!
74+
// By making it private, we enforce it always points to a valid custom_fan_modes_ entry.
75+
```
76+
77+
##### Example: Invariant coupling
78+
79+
```cpp
80+
class Buffer {
81+
public:
82+
void resize(size_t new_size) {
83+
auto new_data = std::make_unique<uint8_t[]>(new_size);
84+
if (this->data_) {
85+
std::memcpy(new_data.get(), this->data_.get(), std::min(this->size_, new_size));
86+
}
87+
this->data_ = std::move(new_data);
88+
this->size_ = new_size; // Must stay in sync with data_
89+
}
90+
91+
private:
92+
// These MUST stay synchronized - making them private prevents:
93+
// this->size_ = 1000; // But data_ is still old allocation - buffer overflow!
94+
std::unique_ptr<uint8_t[]> data_;
95+
size_t size_{0}; // Must match allocated size of data_
96+
};
97+
```
98+
2999
### Use of external libraries
30100

31101
In general, we try to avoid use of external libraries.

0 commit comments

Comments
 (0)