ResInsight/docs/agents/coding-style.md

# Coding Style Guidelines

This document describes the coding style and formatting conventions for ResInsight.

## Code Formatting

### clang-format

ResInsight uses clang-format for C++ code formatting:
- **Configuration**: `.clang-format` file in repository root
- **Version**: Use clang-format-19 to enforce style

### Python Formatting

Python code should be formatted using `ruff`:

```bash
# Format source code
python -m ruff format test_polygons.py 

# Check code style
python -m ruff check --fix test_polygons.py
```

### CMake Formatting

CMake files (`CMakeLists.txt`, `*.cmake`) are formatted with `cmake-format` from the
[`cmakelang`](https://pypi.org/project/cmakelang/) package, using the repository configuration
`cmake/cmake-format.py`. The `cmake-format` GitHub workflow enforces this on every push and opens a
"Fixes by cmake-format" pull request when a file is not formatted.

- **If `cmakelang` is available**, run `cmake-format` in place on any CMake file you changed, so the
  formatting fix lands in your commit instead of a follow-up bot PR:

  ```bash
  cmake-format -c cmake/cmake-format.py -i ApplicationLibCode/FeatureTests/CMakeLists.txt
  ```

  Always pass `-c cmake/cmake-format.py` (the repo config) and `-i` (edit in place). Install with
  `python -m pip install --user cmakelang` if it is missing; if installing is not possible, skip this
  step and rely on the CI workflow.
- On Windows, `cmake-format` writes LF line endings, so `git status` may show a formatted file as
  modified even when the content is unchanged. Use `git diff --quiet -- <file>` to confirm whether
  there is any real formatting drift.

## Copyright Headers

- New files must use the **current year** (the year the file is created) in the copyright header
- Never change the copyright year in existing files — the year reflects when the file was originally created
- Example header for a file created in 2026:

```cpp
/////////////////////////////////////////////////////////////////////////////////
//
//  Copyright (C) 2026-     Equinor ASA
//
```

## Language Standards

- **C++**: C++23 standard
- **Python**: Python 3.11+

## Best Practices

### General Guidelines

1. **Minimal Changes**: Make the smallest possible changes to achieve the goal
2. **Preserve Formatting**: Do not reformat unrelated code
3. **Comments**: Match the style of existing comments in the file; keep all comments short and concise — a brief phrase or single sentence is preferred over a multi-sentence explanation
4. **Libraries**: Use existing libraries whenever possible; only add new libraries or update versions if absolutely necessary

### Code Quality

1. Always validate changes don't break existing behavior
2. Always validate changes don't introduce security vulnerabilities
3. Fix any vulnerabilities related to your changes
4. Run linters, builds and tests before making code changes to understand any existing issues
5. Always try to lint, build and test code changes as soon as possible after making them

### Testing

1. Only run linters, builds and tests that already exist
2. Do not add new linting, building or testing tools unless necessary to fix the issue
3. It is unacceptable to remove or edit unrelated tests
4. Documentation changes do not need to be linted, built or tested unless there are specific tests for documentation

### Header / Implementation Split (C++)

Put function bodies in the `.cpp` file, not inline in the header.

- Declare member functions in the header; define them in the matching `.cpp`.
- Exceptions: function templates that must stay in the header, `constexpr` functions where the compiler requires the definition to be visible, and functions with an empty body (`{}`) or a short initializer list.

```cpp
// OK – short one-line body allowed inline
class RimFoo
{
public:
    bool canAddSubCollection() const override { return false; }
};

// Bad – multi-statement body inline in the header
class RimFoo
{
public:
    bool canAddSubCollection() const override
    {
        if ( m_locked ) return false;
        return m_children.empty();
    }
};

// Good – declared in the header, defined in the .cpp
// RimFoo.h
class RimFoo
{
public:
    bool canAddSubCollection() const override;
};

// RimFoo.cpp
bool RimFoo::canAddSubCollection() const
{
    if ( m_locked ) return false;
    return m_children.empty();
}

// OK – empty body / short initializer list allowed inline
class RimBar
{
public:
    void reset() {}
    RimBar( int value )
        : m_value( value )
    {
    }

private:
    int m_value = 0;
};
```

Why: keeps headers light (faster builds, smaller include surface), keeps the implementation file as the single source of truth for behavior, and matches the existing style across `ApplicationLibCode`.

### Logging

Use `std::format` to build log messages — not `QString(...).arg(...)`.

- `RiaLogging::info` / `warning` / `error` take `std::string_view`. Passing a `QString` requires a trailing `.toStdString()`; `std::format` avoids that and reads better.
- `QString` arguments can be passed directly to `std::format` — include `RiaQStringFormatter.h`, which provides a `std::formatter<QString>` specialization.

```cpp
// Bad – QString with positional placeholders, manual conversion
RiaLogging::info( QString( "Exported %1 cells for well '%2'." ).arg( count ).arg( wellName ).toStdString() );

// Good – std::format, QString passed directly
RiaLogging::info( std::format( "Exported {} cells for well '{}'.", count, wellName ) );
```

### Asserts

Use `CAF_ASSERT` (`cafAssert.h`). Do not add new `CVF_ASSERT`, `CVF_ASSERT_MSG`, `CVF_FAIL_MSG` or `CVF_TIGHT_ASSERT` — those are legacy and are being migrated away from.

An assert documents an invariant that holds whenever the program is correct. It is not error handling. Conditions that can legitimately occur at run time — a missing file, a malformed input deck, a failed network call, a user selecting nothing — must be handled with a return value, `std::optional`, `std::expected` or a logged error message, never with an assert.

```cpp
// Bad – a missing file is a run-time condition, not a broken invariant
CAF_ASSERT( QFile::exists( fileName ) );

// Good – handled and reported
if ( !QFile::exists( fileName ) )
{
    RiaLogging::error( std::format( "File not found: {}", fileName ) );
    return {};
}

// Good – a broken invariant, unreachable unless the code is wrong
CAF_ASSERT( index >= 0 && index < m_values.size() );
```

`CAF_ASSERT` follows the semantics of the standard `assert()`:

- **Debug builds**: active. A failure prints file, line and expression, then calls `std::abort()`.
- **Optimized builds** (Release, RelWithDebInfo): compiled out. The expression is still type checked but never evaluated, so it must be free of side effects — never put work inside the assert that the surrounding code depends on.

Configure with `-DRESINSIGHT_ENABLE_ASSERTS_IN_RELEASE=ON` to keep the asserts active in an optimized build, which is useful when reproducing a problem in RelWithDebInfo with a debugger attached.

A failing assert aborts via `SIGABRT`, which the crash handler installed in `RiaMain.cpp` picks up: the failure is written to the log file and reported to OpenTelemetry with a stack trace.

### Lambda Functions

Keep lambdas short and readable:

- **1–3 lines**: keep inline
- **4+ lines or contains control flow** (if/for/while): extract to a named method (e.g. `onApplyClicked()`) and call it from a single-line lambda

```cpp
// Good – short inline lambda
addNewButton( "Show Report", [this]() { showReport(); } );

// Good – multi-statement, no control flow, still inline
addNewButton( "Clear Data",
              [this]()
              {
                  clearData();
                  updateConnectedEditors();
              } );

// Good – complex logic extracted to a named method
addNewButton( "Apply", [this]() { onApplyClicked(); } );
void MyClass::onApplyClicked() { /* complex logic here */ }
```

### Context Menus

When adding or modifying context-menu items for a PDM object, override `appendMenuItems( caf::CmdFeatureMenuBuilder& menuBuilder )` on that object and add the entries there.

- Do **not** modify `RimContextCommandBuilder` to add object-specific menu items. It dispatches to each object's `appendMenuItems`; keep object-specific logic in the object.

```cpp
// Good – menu items added in the object's own appendMenuItems override
void RimFoo::appendMenuItems( caf::CmdFeatureMenuBuilder& menuBuilder ) const
{
    menuBuilder << "RicSomeFeature";
}
```

## OpenMP

ResInsight builds with MSVC `/openmp`, which is **OpenMP 2.0**. Loop indices must be a signed
integer type, and `collapse` and min/max reductions are not available.

### Never write an orphaned work-sharing construct

A `#pragma omp for` must have a lexically enclosing `#pragma omp parallel` in the same function.
An orphaned `#pragma omp for` is not merely useless - it can deadlock the application.

The reason is that the OpenMP master thread is also the Qt GUI thread. When the GUI thread is
inside a parallel region and a progress update or a repaint runs, Qt work executes directly on
that thread, which pulls rendering code into the dynamic extent of an active parallel team. An
orphaned `#pragma omp for` reached that way is encountered by one thread of a team whose other
threads never reach it, and the implicit barrier at the end of the construct never completes.

```cpp
// Bad - deadlocks if this function is reached from inside an active parallel region
void toTextureImageRegion( ... )
{
#pragma omp for
    for ( int y = 0; y < sizeY; ++y )
```

### Collect per thread, merge afterwards

Do not guard a `push_back` into a shared container with `#pragma omp critical` inside a hot loop.
Give each thread its own buffer and merge once the region has ended. Merging in thread order also
makes the result reproducible from run to run, because the default static schedule assigns
contiguous chunks to the threads.

```cpp
// Good
const int                        numberOfThreads = RiaOpenMPTools::availableThreadCount();
std::vector<std::vector<size_t>> threadCells( numberOfThreads );

#pragma omp parallel
{
    const int myThread = RiaOpenMPTools::currentThreadIndex();

    // NB! We are inside a parallel section, do not use "parallel for" here
#pragma omp for
    for ( int i = 0; i < cellCount; i++ )
    {
        if ( isIncluded( i ) ) threadCells[myThread].push_back( i );
    }
}

for ( const auto& cellsForThread : threadCells )
{
    cells.insert( cells.end(), cellsForThread.begin(), cellsForThread.end() );
}
```

No synchronization is needed when each iteration writes to its own element of a container that was
sized before the loop. Do not add `critical` for that case.

### Name every critical section

All unnamed `#pragma omp critical` regions share one process-wide lock, so unrelated subsystems
serialize against each other. Use the `critical_section_<purpose>` convention instead.

### Exceptions and the GUI

An exception that escapes an OpenMP structured block terminates the process on MSVC instead of
unwinding, so catch inside the loop body. Touch Qt widgets only from the thread owning them; use
`RiaThreadSafeLogger` or a queued connection to hand data back.

## Commit Conventions

When creating commits:
- Use issue number at the start of the title: `#12773 Python: Add API for creating valve templates`
- Follow git conventions for commit messages
- Keep commit messages short and concise: a one-line title plus only as much body as needed to explain the "why"
- Always run python formatting/check on changed files before commits
- When you change a CMake file and `cmakelang` is available, run `cmake-format` on it before committing (see [CMake Formatting](#cmake-formatting))