opm/models/blackoil: address clang-tidy warnings#7098
Conversation
to lower congnitive load
to lower congnitive load
akva2
added
the
manual:irrelevant
akva2
changed the title
There was a problem hiding this comment.
Pull request overview
Refactors multiple BlackOil *Params initialization paths to address clang-tidy warnings around long methods / cognitive complexity by extracting validation and per-keyword processing into smaller helpers.
Changes:
- Split large
initFromState()implementations intoverifyState()helpers (anonymous namespace) and multipleprocess*()/setup*()member functions. - Move internal sizing/assignment helpers (e.g.,
setNumSatRegions,setNumMixRegions,setPlyrock) intoprivate:to reduce surface area. - Minor cleanups in initialization and table parsing (e.g., SDENSITY parsing helper for ExtBO).
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| opm/models/blackoil/blackoilsolventparams.hpp | Makes internal helper methods private and adds declarations for extracted setup/processing helpers. |
| opm/models/blackoil/blackoilsolventparams.cpp | Extracts solvent init logic into smaller functions; adds shared state verification; refactors miscible-table processing. |
| opm/models/blackoil/blackoilpolymerparams.hpp | Moves internal sizing/initialization helpers to private: and declares extracted processing helpers. |
| opm/models/blackoil/blackoilpolymerparams.cpp | Extracts polymer init logic into smaller functions; adds shared state verification; refactors keyword/table handling. |
| opm/models/blackoil/blackoilextboparams.cpp | Extracts ExtBO state verification and SDENSITY parsing into helpers. |
| opm/models/blackoil/blackoilbrineparams.cpp | Extracts brine state verification helper and keeps init logic intact. |
| opm/models/blackoil/blackoilbioeffectsparams.cpp | Extracts bioeffects/MICP state verification helper. |
Comments suppressed due to low confidence (1)
opm/models/blackoil/blackoilpolymerparams.cpp:39
std::logis used in this file (e.g., inprocessPlyshlog) but there is no direct#include <cmath>. Relying on transitive includes can break builds on some standard libraries/compilers; please include<cmath>explicitly.
#include <algorithm>
#include <cassert>
#include <cstddef>
#include <stdexcept>
#include <type_traits>
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| else { | ||
| // if empty keyword. Try to use the pmisc table as default. | ||
| if (!pmisc_.empty()) { | ||
| tlPMixTable_ = pmisc_; | ||
| } | ||
| else { | ||
| OpmLog::warning("If the pressure dependent TL values in " | ||
| "TLPMIXPA is defaulted (no entries), then " | ||
| "the PMISC tables must be specified."); | ||
| // default | ||
| const std::vector<double> x = {0.0,1.0e20}; | ||
| const std::vector<double> y = {1.0,1.0}; | ||
| const TabulatedFunction ones = TabulatedFunction(2, x, y); | ||
| for (unsigned regionIdx = 0; regionIdx < numMiscRegions; ++regionIdx) { | ||
| tlPMixTable_[regionIdx] = ones; | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Problem is tableManager.hasTables("TLMPIXPA") returns false it the table is empty, so it does not let me distinguish between no entry and entry with empty table.
There was a problem hiding this comment.
Point 1. This is probably functionality nobody uses anymore. 2. The fallback on using pmisc is questionable. I think your suggestion to make the default constant is okay. In practice, this means the keyword is ignored, so adding a warning is probably a good idea. Or maybe even better is to have no defaults and throw if empty.
There was a problem hiding this comment.
Since you seem to agree, I removed the pmisc usage and emit a warning that dummy values are used. This was the most pragmatic approach to fix the regression failure.
to lower congnitive load
akva2
force-pushed
the
blackoilmodel_clang_tidy
branch
from
4251885 to
b41972f
Compare
to lower congnitive load
to lower congnitive load
to lower congnitive load
to lower congnitive load
to lower congnitive load
to lower congnitive load
to lower congnitive load
to lower congnitive load
to lower congnitive load
to lower congnitive load
to lower congnitive load
to lower congnitive load
to lower congnitive load
to lower congnitive load
to lower cognitive load
to lower congnitive load
to lower congnitive load
to lower congnitive load
to lower congnitive load
to lower congnitive load
to lower congnitive load
to lower congnitive load
to lower congnitive load
akva2
force-pushed
the
blackoilmodel_clang_tidy
branch
from
b41972f to
03dc5c8
Compare
|
@totto82 can i have your opinion on the tlpmixpa woes please (see PR description, and still-unresolved copilot comment). |
|
jenkins build this please |
to lower congnitive load
akva2
force-pushed
the
blackoilmodel_clang_tidy
branch
from
03dc5c8 to
295154f
Compare
|
jenkins build this please |
These are all about long methods / "high cognitive complexity".
The last commit needs special attention. I found a bug/issue, and have to tried to resolve as good as I can.
The original code used deck keyword existence and empty keyword state. We no longer have the tools to distinguish these at this point. I've tried to join the two elses.