Skip to content

Remove Mesh::load#3212

Draft
ZedThree wants to merge 4 commits intonextfrom
remove-mesh-load
Draft

Remove Mesh::load#3212
ZedThree wants to merge 4 commits intonextfrom
remove-mesh-load

Conversation

@ZedThree
Copy link
Member

@ZedThree ZedThree commented Nov 24, 2025

Mesh should be entirely created in constructor

This is a little bit experimental -- the reason we have Mesh::load in the first place is to break the circular dependency between Mesh and Coordinates, and the reason we construct Coordinates in BoutMesh::BoutMesh is to ensure any communication required in Coordinates is properly synchronised. So we might just end up replacing calls to Mesh::load with calls to Mesh::getCoordinates, but at least the Mesh constructor will be atomic

@ZedThree
Remove Mesh::load
1c2ef26 
`Mesh` should be entirely created in constructor
github-actions[bot]
github-actions bot reviewed Nov 24, 2025
View reviewed changes
Copy link
Contributor

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

clang-tidy made some suggestions

src/mesh/impls/bout/boutmesh.cxx

BoutMesh::BoutMesh(GridDataSource* s, Options* opt) : Mesh(s, opt) {
TRACE("BoutMesh::BoutMesh()");
output_progress << _("Loading mesh") << endl;
Copy link
Contributor

@github-actions github-actions bot Nov 24, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

warning: do not use 'endl' with streams; use '\n' instead [performance-avoid-endl]

Suggested change
output_progress << _("Loading mesh") << endl;
output_progress << _("Loading mesh") << '\n';

src/mesh/impls/bout/boutmesh.cxx

BoutMesh::BoutMesh(GridDataSource* s, Options* opt) : Mesh(s, opt) {
TRACE("BoutMesh::BoutMesh()");
output_progress << _("Loading mesh") << endl;
Copy link
Contributor

@github-actions github-actions bot Nov 24, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

warning: no header providing "_" is directly included [misc-include-cleaner]

src/mesh/impls/bout/boutmesh.cxx:28:

- #include <bout/boundary_region.hxx>
+ #include "bout/sys/gettext.hxx"
+ #include <bout/boundary_region.hxx>

src/mesh/impls/bout/boutmesh.cxx

OPTION(options, MZ, 64);
OPTION(options, nz, MZ);
ASSERT0(nz == MZ);
Copy link
Contributor

@github-actions github-actions bot Nov 24, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

warning: no header providing "ASSERT0" is directly included [misc-include-cleaner]

src/mesh/impls/bout/boutmesh.cxx:28:

- #include <bout/boundary_region.hxx>
+ #include "bout/assert.hxx"
+ #include <bout/boundary_region.hxx>

src/mesh/impls/bout/boutmesh.cxx
output_info.write(_("\tRead nz from input grid file\n"));
}

output_info << _("\tGrid size: ") << nx << " x " << ny << " x " << nz << endl;
Copy link
Contributor

@github-actions github-actions bot Nov 24, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

warning: do not use 'endl' with streams; use '\n' instead [performance-avoid-endl]

Suggested change
output_info << _("\tGrid size: ") << nx << " x " << ny << " x " << nz << endl;
output_info << _("\tGrid size: ") << nx << " x " << ny << " x " << nz << '\n';

src/mesh/impls/bout/boutmesh.cxx
NZPE = 1;

output_info << _("\tGuard cells (x,y,z): ") << MXG << ", " << MYG << ", " << MZG
<< std::endl;
Copy link
Contributor

@github-actions github-actions bot Nov 24, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

warning: do not use 'std::endl' with streams; use '\n' instead [performance-avoid-endl]

Suggested change
<< std::endl;
<< '\n';

src/mesh/impls/bout/boutmesh.cxx Outdated
{
int mybndry = static_cast<int>(!(iterateBndryLowerY().isDone()));
int allbndry = 0;
mpi->MPI_Allreduce(&mybndry, &allbndry, 1, MPI_INT, MPI_BOR, getXcomm(yend));
Copy link
Contributor

@github-actions github-actions bot Nov 24, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

warning: Call to virtual method 'BoutMesh::getXcomm' during construction bypasses virtual dispatch [clang-analyzer-optin.cplusplus.VirtualCall]

    mpi->MPI_Allreduce(&mybndry, &allbndry, 1, MPI_INT, MPI_BOR, getXcomm(yend));
                                                                 ^
Additional context

src/mesh/impls/bout/boutmesh.cxx:56: Assuming the condition is false

  if (!options->isSet("symmetricGlobalY")) {
      ^

src/mesh/impls/bout/boutmesh.cxx:56: Taking false branch

  if (!options->isSet("symmetricGlobalY")) {
  ^

src/mesh/impls/bout/boutmesh.cxx:83: Assuming the condition is false

  if (Mesh::get(nx, "nx") != 0) {
      ^

src/mesh/impls/bout/boutmesh.cxx:83: Taking false branch

  if (Mesh::get(nx, "nx") != 0) {
  ^

src/mesh/impls/bout/boutmesh.cxx:87: Assuming the condition is false

  if (Mesh::get(ny, "ny") != 0) {
      ^

src/mesh/impls/bout/boutmesh.cxx:87: Taking false branch

  if (Mesh::get(ny, "ny") != 0) {
  ^

src/mesh/impls/bout/boutmesh.cxx:91: Assuming the condition is false

  if (Mesh::get(nz, "nz") != 0) {
      ^

src/mesh/impls/bout/boutmesh.cxx:91: Taking false branch

  if (Mesh::get(nz, "nz") != 0) {
  ^

src/mesh/impls/bout/boutmesh.cxx:114: Assuming the condition is false

  if (Mesh::get(MXG, "MXG") != 0) {
      ^

src/mesh/impls/bout/boutmesh.cxx:114: Taking false branch

  if (Mesh::get(MXG, "MXG") != 0) {
  ^

src/mesh/impls/bout/boutmesh.cxx:118: Assuming field 'MXG' is >= 0

  ASSERT0(MXG >= 0);
          ^

include/bout/assert.hxx:29: expanded from macro 'ASSERT0'

  if (!(condition)) {                                                                    \
        ^

src/mesh/impls/bout/boutmesh.cxx:118: Taking false branch

  ASSERT0(MXG >= 0);
  ^

include/bout/assert.hxx:29: expanded from macro 'ASSERT0'

  if (!(condition)) {                                                                    \
  ^

src/mesh/impls/bout/boutmesh.cxx:120: Assuming the condition is false

  if (Mesh::get(MYG, "MYG") != 0) {
      ^

src/mesh/impls/bout/boutmesh.cxx:120: Taking false branch

  if (Mesh::get(MYG, "MYG") != 0) {
  ^

src/mesh/impls/bout/boutmesh.cxx:123: Assuming field 'MYG' is >= 0

  ASSERT0(MYG >= 0);
          ^

include/bout/assert.hxx:29: expanded from macro 'ASSERT0'

  if (!(condition)) {                                                                    \
        ^

src/mesh/impls/bout/boutmesh.cxx:123: Taking false branch

  ASSERT0(MYG >= 0);
  ^

include/bout/assert.hxx:29: expanded from macro 'ASSERT0'

  if (!(condition)) {                                                                    \
  ^

src/mesh/impls/bout/boutmesh.cxx:127: Field 'MZG' is >= 0

  ASSERT0(MZG >= 0);
          ^

src/mesh/impls/bout/boutmesh.cxx:127: Taking false branch

  ASSERT0(MZG >= 0);
  ^

include/bout/assert.hxx:29: expanded from macro 'ASSERT0'

  if (!(condition)) {                                                                    \
  ^

src/mesh/impls/bout/boutmesh.cxx:147: Assuming the condition is false

  if (options.isSet("NXPE") or options.isSet("NYPE")) {
      ^

src/mesh/impls/bout/boutmesh.cxx:147: Left side of '||' is false

  if (options.isSet("NXPE") or options.isSet("NYPE")) {
      ^

src/mesh/impls/bout/boutmesh.cxx:147: Assuming the condition is true

  if (options.isSet("NXPE") or options.isSet("NYPE")) {
      ^

src/mesh/impls/bout/boutmesh.cxx:147: Taking true branch

  if (options.isSet("NXPE") or options.isSet("NYPE")) {
  ^

src/mesh/impls/bout/boutmesh.cxx:168: Assuming the condition is false

  if (options.isSet("zperiod")) {
      ^

src/mesh/impls/bout/boutmesh.cxx:168: Taking false branch

  if (options.isSet("zperiod")) {
  ^

src/mesh/impls/bout/boutmesh.cxx:194: Assuming the condition is false

  if (!source->get(this, ShiftAngle, "ShiftAngle", LocalNx, getGlobalXIndex(0))) {
      ^

src/mesh/impls/bout/boutmesh.cxx:194: Taking false branch

  if (!source->get(this, ShiftAngle, "ShiftAngle", LocalNx, getGlobalXIndex(0))) {
  ^

src/mesh/impls/bout/boutmesh.cxx:198: Assuming field 'TwistShift' is false

  if (TwistShift) {
      ^

src/mesh/impls/bout/boutmesh.cxx:198: Taking false branch

  if (TwistShift) {
  ^

src/mesh/impls/bout/boutmesh.cxx:218: Assuming the condition is true

  if (possible_boundaries.empty()) {
      ^

src/mesh/impls/bout/boutmesh.cxx:218: Taking true branch

  if (possible_boundaries.empty()) {
  ^

src/mesh/impls/bout/boutmesh.cxx:227: Assuming the condition is false

  if (!boundary.empty()) {
      ^

src/mesh/impls/bout/boutmesh.cxx:227: Taking false branch

  if (!boundary.empty()) {
  ^

src/mesh/impls/bout/boutmesh.cxx:245: Assuming the condition is false

    int mybndry = static_cast<int>(!(iterateBndryLowerY().isDone()));
                                   ^

src/mesh/impls/bout/boutmesh.cxx:247: Call to virtual method 'BoutMesh::getXcomm' during construction bypasses virtual dispatch

    mpi->MPI_Allreduce(&mybndry, &allbndry, 1, MPI_INT, MPI_BOR, getXcomm(yend));
                                                                 ^

src/mesh/impls/bout/boutmesh.cxx Outdated
has_boundary_lower_y = static_cast<bool>(allbndry);
}
{
int mybndry = static_cast<int>(!(iterateBndryUpperY().isDone()));
Copy link
Contributor

@github-actions github-actions bot Nov 24, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

warning: Call to virtual method 'BoutMesh::iterateBndryUpperY' during construction bypasses virtual dispatch [clang-analyzer-optin.cplusplus.VirtualCall]

    int mybndry = static_cast<int>(!(iterateBndryUpperY().isDone()));
                                     ^
Additional context

src/mesh/impls/bout/boutmesh.cxx:56: Assuming the condition is false

  if (!options->isSet("symmetricGlobalY")) {
      ^

src/mesh/impls/bout/boutmesh.cxx:56: Taking false branch

  if (!options->isSet("symmetricGlobalY")) {
  ^

src/mesh/impls/bout/boutmesh.cxx:83: Assuming the condition is false

  if (Mesh::get(nx, "nx") != 0) {
      ^

src/mesh/impls/bout/boutmesh.cxx:83: Taking false branch

  if (Mesh::get(nx, "nx") != 0) {
  ^

src/mesh/impls/bout/boutmesh.cxx:87: Assuming the condition is false

  if (Mesh::get(ny, "ny") != 0) {
      ^

src/mesh/impls/bout/boutmesh.cxx:87: Taking false branch

  if (Mesh::get(ny, "ny") != 0) {
  ^

src/mesh/impls/bout/boutmesh.cxx:91: Assuming the condition is false

  if (Mesh::get(nz, "nz") != 0) {
      ^

src/mesh/impls/bout/boutmesh.cxx:91: Taking false branch

  if (Mesh::get(nz, "nz") != 0) {
  ^

src/mesh/impls/bout/boutmesh.cxx:114: Assuming the condition is false

  if (Mesh::get(MXG, "MXG") != 0) {
      ^

src/mesh/impls/bout/boutmesh.cxx:114: Taking false branch

  if (Mesh::get(MXG, "MXG") != 0) {
  ^

src/mesh/impls/bout/boutmesh.cxx:118: Assuming field 'MXG' is >= 0

  ASSERT0(MXG >= 0);
          ^

include/bout/assert.hxx:29: expanded from macro 'ASSERT0'

  if (!(condition)) {                                                                    \
        ^

src/mesh/impls/bout/boutmesh.cxx:118: Taking false branch

  ASSERT0(MXG >= 0);
  ^

include/bout/assert.hxx:29: expanded from macro 'ASSERT0'

  if (!(condition)) {                                                                    \
  ^

src/mesh/impls/bout/boutmesh.cxx:120: Assuming the condition is false

  if (Mesh::get(MYG, "MYG") != 0) {
      ^

src/mesh/impls/bout/boutmesh.cxx:120: Taking false branch

  if (Mesh::get(MYG, "MYG") != 0) {
  ^

src/mesh/impls/bout/boutmesh.cxx:123: Assuming field 'MYG' is >= 0

  ASSERT0(MYG >= 0);
          ^

include/bout/assert.hxx:29: expanded from macro 'ASSERT0'

  if (!(condition)) {                                                                    \
        ^

src/mesh/impls/bout/boutmesh.cxx:123: Taking false branch

  ASSERT0(MYG >= 0);
  ^

include/bout/assert.hxx:29: expanded from macro 'ASSERT0'

  if (!(condition)) {                                                                    \
  ^

src/mesh/impls/bout/boutmesh.cxx:127: Field 'MZG' is >= 0

  ASSERT0(MZG >= 0);
          ^

src/mesh/impls/bout/boutmesh.cxx:127: Taking false branch

  ASSERT0(MZG >= 0);
  ^

include/bout/assert.hxx:29: expanded from macro 'ASSERT0'

  if (!(condition)) {                                                                    \
  ^

src/mesh/impls/bout/boutmesh.cxx:147: Assuming the condition is false

  if (options.isSet("NXPE") or options.isSet("NYPE")) {
      ^

src/mesh/impls/bout/boutmesh.cxx:147: Left side of '||' is false

  if (options.isSet("NXPE") or options.isSet("NYPE")) {
      ^

src/mesh/impls/bout/boutmesh.cxx:147: Assuming the condition is true

  if (options.isSet("NXPE") or options.isSet("NYPE")) {
      ^

src/mesh/impls/bout/boutmesh.cxx:147: Taking true branch

  if (options.isSet("NXPE") or options.isSet("NYPE")) {
  ^

src/mesh/impls/bout/boutmesh.cxx:168: Assuming the condition is false

  if (options.isSet("zperiod")) {
      ^

src/mesh/impls/bout/boutmesh.cxx:168: Taking false branch

  if (options.isSet("zperiod")) {
  ^

src/mesh/impls/bout/boutmesh.cxx:194: Assuming the condition is false

  if (!source->get(this, ShiftAngle, "ShiftAngle", LocalNx, getGlobalXIndex(0))) {
      ^

src/mesh/impls/bout/boutmesh.cxx:194: Taking false branch

  if (!source->get(this, ShiftAngle, "ShiftAngle", LocalNx, getGlobalXIndex(0))) {
  ^

src/mesh/impls/bout/boutmesh.cxx:198: Assuming field 'TwistShift' is false

  if (TwistShift) {
      ^

src/mesh/impls/bout/boutmesh.cxx:198: Taking false branch

  if (TwistShift) {
  ^

src/mesh/impls/bout/boutmesh.cxx:218: Assuming the condition is true

  if (possible_boundaries.empty()) {
      ^

src/mesh/impls/bout/boutmesh.cxx:218: Taking true branch

  if (possible_boundaries.empty()) {
  ^

src/mesh/impls/bout/boutmesh.cxx:227: Assuming the condition is false

  if (!boundary.empty()) {
      ^

src/mesh/impls/bout/boutmesh.cxx:227: Taking false branch

  if (!boundary.empty()) {
  ^

src/mesh/impls/bout/boutmesh.cxx:245: Assuming the condition is false

    int mybndry = static_cast<int>(!(iterateBndryLowerY().isDone()));
                                   ^

src/mesh/impls/bout/boutmesh.cxx:251: Call to virtual method 'BoutMesh::iterateBndryUpperY' during construction bypasses virtual dispatch

    int mybndry = static_cast<int>(!(iterateBndryUpperY().isDone()));
                                     ^

src/mesh/impls/bout/boutmesh.cxx Outdated
{
int mybndry = static_cast<int>(!(iterateBndryUpperY().isDone()));
int allbndry = 0;
mpi->MPI_Allreduce(&mybndry, &allbndry, 1, MPI_INT, MPI_BOR, getXcomm(ystart));
Copy link
Contributor

@github-actions github-actions bot Nov 24, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

warning: Call to virtual method 'BoutMesh::getXcomm' during construction bypasses virtual dispatch [clang-analyzer-optin.cplusplus.VirtualCall]

    mpi->MPI_Allreduce(&mybndry, &allbndry, 1, MPI_INT, MPI_BOR, getXcomm(ystart));
                                                                 ^
Additional context

src/mesh/impls/bout/boutmesh.cxx:56: Assuming the condition is false

  if (!options->isSet("symmetricGlobalY")) {
      ^

src/mesh/impls/bout/boutmesh.cxx:56: Taking false branch

  if (!options->isSet("symmetricGlobalY")) {
  ^

src/mesh/impls/bout/boutmesh.cxx:83: Assuming the condition is false

  if (Mesh::get(nx, "nx") != 0) {
      ^

src/mesh/impls/bout/boutmesh.cxx:83: Taking false branch

  if (Mesh::get(nx, "nx") != 0) {
  ^

src/mesh/impls/bout/boutmesh.cxx:87: Assuming the condition is false

  if (Mesh::get(ny, "ny") != 0) {
      ^

src/mesh/impls/bout/boutmesh.cxx:87: Taking false branch

  if (Mesh::get(ny, "ny") != 0) {
  ^

src/mesh/impls/bout/boutmesh.cxx:91: Assuming the condition is false

  if (Mesh::get(nz, "nz") != 0) {
      ^

src/mesh/impls/bout/boutmesh.cxx:91: Taking false branch

  if (Mesh::get(nz, "nz") != 0) {
  ^

src/mesh/impls/bout/boutmesh.cxx:114: Assuming the condition is false

  if (Mesh::get(MXG, "MXG") != 0) {
      ^

src/mesh/impls/bout/boutmesh.cxx:114: Taking false branch

  if (Mesh::get(MXG, "MXG") != 0) {
  ^

src/mesh/impls/bout/boutmesh.cxx:118: Assuming field 'MXG' is >= 0

  ASSERT0(MXG >= 0);
          ^

include/bout/assert.hxx:29: expanded from macro 'ASSERT0'

  if (!(condition)) {                                                                    \
        ^

src/mesh/impls/bout/boutmesh.cxx:118: Taking false branch

  ASSERT0(MXG >= 0);
  ^

include/bout/assert.hxx:29: expanded from macro 'ASSERT0'

  if (!(condition)) {                                                                    \
  ^

src/mesh/impls/bout/boutmesh.cxx:120: Assuming the condition is false

  if (Mesh::get(MYG, "MYG") != 0) {
      ^

src/mesh/impls/bout/boutmesh.cxx:120: Taking false branch

  if (Mesh::get(MYG, "MYG") != 0) {
  ^

src/mesh/impls/bout/boutmesh.cxx:123: Assuming field 'MYG' is >= 0

  ASSERT0(MYG >= 0);
          ^

include/bout/assert.hxx:29: expanded from macro 'ASSERT0'

  if (!(condition)) {                                                                    \
        ^

src/mesh/impls/bout/boutmesh.cxx:123: Taking false branch

  ASSERT0(MYG >= 0);
  ^

include/bout/assert.hxx:29: expanded from macro 'ASSERT0'

  if (!(condition)) {                                                                    \
  ^

src/mesh/impls/bout/boutmesh.cxx:127: Field 'MZG' is >= 0

  ASSERT0(MZG >= 0);
          ^

src/mesh/impls/bout/boutmesh.cxx:127: Taking false branch

  ASSERT0(MZG >= 0);
  ^

include/bout/assert.hxx:29: expanded from macro 'ASSERT0'

  if (!(condition)) {                                                                    \
  ^

src/mesh/impls/bout/boutmesh.cxx:147: Assuming the condition is false

  if (options.isSet("NXPE") or options.isSet("NYPE")) {
      ^

src/mesh/impls/bout/boutmesh.cxx:147: Left side of '||' is false

  if (options.isSet("NXPE") or options.isSet("NYPE")) {
      ^

src/mesh/impls/bout/boutmesh.cxx:147: Assuming the condition is true

  if (options.isSet("NXPE") or options.isSet("NYPE")) {
      ^

src/mesh/impls/bout/boutmesh.cxx:147: Taking true branch

  if (options.isSet("NXPE") or options.isSet("NYPE")) {
  ^

src/mesh/impls/bout/boutmesh.cxx:168: Assuming the condition is false

  if (options.isSet("zperiod")) {
      ^

src/mesh/impls/bout/boutmesh.cxx:168: Taking false branch

  if (options.isSet("zperiod")) {
  ^

src/mesh/impls/bout/boutmesh.cxx:194: Assuming the condition is false

  if (!source->get(this, ShiftAngle, "ShiftAngle", LocalNx, getGlobalXIndex(0))) {
      ^

src/mesh/impls/bout/boutmesh.cxx:194: Taking false branch

  if (!source->get(this, ShiftAngle, "ShiftAngle", LocalNx, getGlobalXIndex(0))) {
  ^

src/mesh/impls/bout/boutmesh.cxx:198: Assuming field 'TwistShift' is false

  if (TwistShift) {
      ^

src/mesh/impls/bout/boutmesh.cxx:198: Taking false branch

  if (TwistShift) {
  ^

src/mesh/impls/bout/boutmesh.cxx:218: Assuming the condition is true

  if (possible_boundaries.empty()) {
      ^

src/mesh/impls/bout/boutmesh.cxx:218: Taking true branch

  if (possible_boundaries.empty()) {
  ^

src/mesh/impls/bout/boutmesh.cxx:227: Assuming the condition is false

  if (!boundary.empty()) {
      ^

src/mesh/impls/bout/boutmesh.cxx:227: Taking false branch

  if (!boundary.empty()) {
  ^

src/mesh/impls/bout/boutmesh.cxx:245: Assuming the condition is false

    int mybndry = static_cast<int>(!(iterateBndryLowerY().isDone()));
                                   ^

src/mesh/impls/bout/boutmesh.cxx:251: Assuming the condition is false

    int mybndry = static_cast<int>(!(iterateBndryUpperY().isDone()));
                                   ^

src/mesh/impls/bout/boutmesh.cxx:253: Call to virtual method 'BoutMesh::getXcomm' during construction bypasses virtual dispatch

    mpi->MPI_Allreduce(&mybndry, &allbndry, 1, MPI_INT, MPI_BOR, getXcomm(ystart));
                                                                 ^

src/mesh/impls/bout/boutmesh.cxx
mpi->MPI_Irecv(buffer, size, PVEC_REAL_MPI_TYPE, proc, tag,
BoutComm::get(), ch->request);
mpi->MPI_Irecv(buffer, size, PVEC_REAL_MPI_TYPE, proc, tag, BoutComm::get(),
ch->request);
Copy link
Contributor

@github-actions github-actions bot Nov 24, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

warning: do not implicitly decay an array into a pointer; consider using gsl::array_view or an explicit cast instead [cppcoreguidelines-pro-bounds-array-to-pointer-decay]

                 ch->request);
                 ^

src/mesh/impls/bout/boutmesh.cxx
mpi->MPI_Irecv(buffer, size, PVEC_REAL_MPI_TYPE, proc, tag,
BoutComm::get(), ch->request);
mpi->MPI_Irecv(buffer, size, PVEC_REAL_MPI_TYPE, proc, tag, BoutComm::get(),
ch->request);
Copy link
Contributor

@github-actions github-actions bot Nov 24, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

warning: do not implicitly decay an array into a pointer; consider using gsl::array_view or an explicit cast instead [cppcoreguidelines-pro-bounds-array-to-pointer-decay]

                 ch->request);
                 ^

github-actions[bot]
github-actions bot reviewed Nov 24, 2025
View reviewed changes
Copy link
Contributor

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

clang-tidy made some suggestions

src/mesh/impls/bout/boutmesh.cxx

BoutMesh::BoutMesh(GridDataSource* s, Options* opt) : Mesh(s, opt) {
TRACE("BoutMesh::BoutMesh()");
output_progress << _("Loading mesh") << endl;
Copy link
Contributor

@github-actions github-actions bot Nov 24, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

warning: do not use 'endl' with streams; use '\n' instead [performance-avoid-endl]

Suggested change
output_progress << _("Loading mesh") << endl;
output_progress << _("Loading mesh") << '\n';

src/mesh/impls/bout/boutmesh.cxx

BoutMesh::BoutMesh(GridDataSource* s, Options* opt) : Mesh(s, opt) {
TRACE("BoutMesh::BoutMesh()");
output_progress << _("Loading mesh") << endl;
Copy link
Contributor

@github-actions github-actions bot Nov 24, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

warning: no header providing "_" is directly included [misc-include-cleaner]

src/mesh/impls/bout/boutmesh.cxx:28:

- #include <bout/boundary_region.hxx>
+ #include "bout/sys/gettext.hxx"
+ #include <bout/boundary_region.hxx>

src/mesh/impls/bout/boutmesh.cxx

OPTION(options, MZ, 64);
OPTION(options, nz, MZ);
ASSERT0(nz == MZ);
Copy link
Contributor

@github-actions github-actions bot Nov 24, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

warning: no header providing "ASSERT0" is directly included [misc-include-cleaner]

src/mesh/impls/bout/boutmesh.cxx:28:

- #include <bout/boundary_region.hxx>
+ #include "bout/assert.hxx"
+ #include <bout/boundary_region.hxx>

src/mesh/impls/bout/boutmesh.cxx
output_info.write(_("\tRead nz from input grid file\n"));
}

output_info << _("\tGrid size: ") << nx << " x " << ny << " x " << nz << endl;
Copy link
Contributor

@github-actions github-actions bot Nov 24, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

warning: do not use 'endl' with streams; use '\n' instead [performance-avoid-endl]

Suggested change
output_info << _("\tGrid size: ") << nx << " x " << ny << " x " << nz << endl;
output_info << _("\tGrid size: ") << nx << " x " << ny << " x " << nz << '\n';

src/mesh/impls/bout/boutmesh.cxx
NZPE = 1;

output_info << _("\tGuard cells (x,y,z): ") << MXG << ", " << MYG << ", " << MZG
<< std::endl;
Copy link
Contributor

@github-actions github-actions bot Nov 24, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

warning: do not use 'std::endl' with streams; use '\n' instead [performance-avoid-endl]

Suggested change
<< std::endl;
<< '\n';

src/mesh/impls/bout/boutmesh.cxx Outdated
{
int mybndry = static_cast<int>(!(iterateBndryLowerY().isDone()));
int allbndry = 0;
mpi->MPI_Allreduce(&mybndry, &allbndry, 1, MPI_INT, MPI_BOR, getXcomm(yend));
Copy link
Contributor

@github-actions github-actions bot Nov 24, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

warning: Call to virtual method 'BoutMesh::getXcomm' during construction bypasses virtual dispatch [clang-analyzer-optin.cplusplus.VirtualCall]

    mpi->MPI_Allreduce(&mybndry, &allbndry, 1, MPI_INT, MPI_BOR, getXcomm(yend));
                                                                 ^
Additional context

src/mesh/impls/bout/boutmesh.cxx:56: Assuming the condition is false

  if (!options->isSet("symmetricGlobalY")) {
      ^

src/mesh/impls/bout/boutmesh.cxx:56: Taking false branch

  if (!options->isSet("symmetricGlobalY")) {
  ^

src/mesh/impls/bout/boutmesh.cxx:83: Assuming the condition is false

  if (Mesh::get(nx, "nx") != 0) {
      ^

src/mesh/impls/bout/boutmesh.cxx:83: Taking false branch

  if (Mesh::get(nx, "nx") != 0) {
  ^

src/mesh/impls/bout/boutmesh.cxx:87: Assuming the condition is false

  if (Mesh::get(ny, "ny") != 0) {
      ^

src/mesh/impls/bout/boutmesh.cxx:87: Taking false branch

  if (Mesh::get(ny, "ny") != 0) {
  ^

src/mesh/impls/bout/boutmesh.cxx:91: Assuming the condition is false

  if (Mesh::get(nz, "nz") != 0) {
      ^

src/mesh/impls/bout/boutmesh.cxx:91: Taking false branch

  if (Mesh::get(nz, "nz") != 0) {
  ^

src/mesh/impls/bout/boutmesh.cxx:114: Assuming the condition is false

  if (Mesh::get(MXG, "MXG") != 0) {
      ^

src/mesh/impls/bout/boutmesh.cxx:114: Taking false branch

  if (Mesh::get(MXG, "MXG") != 0) {
  ^

src/mesh/impls/bout/boutmesh.cxx:118: Assuming field 'MXG' is >= 0

  ASSERT0(MXG >= 0);
          ^

include/bout/assert.hxx:29: expanded from macro 'ASSERT0'

  if (!(condition)) {                                                                    \
        ^

src/mesh/impls/bout/boutmesh.cxx:118: Taking false branch

  ASSERT0(MXG >= 0);
  ^

include/bout/assert.hxx:29: expanded from macro 'ASSERT0'

  if (!(condition)) {                                                                    \
  ^

src/mesh/impls/bout/boutmesh.cxx:120: Assuming the condition is false

  if (Mesh::get(MYG, "MYG") != 0) {
      ^

src/mesh/impls/bout/boutmesh.cxx:120: Taking false branch

  if (Mesh::get(MYG, "MYG") != 0) {
  ^

src/mesh/impls/bout/boutmesh.cxx:123: Assuming field 'MYG' is >= 0

  ASSERT0(MYG >= 0);
          ^

include/bout/assert.hxx:29: expanded from macro 'ASSERT0'

  if (!(condition)) {                                                                    \
        ^

src/mesh/impls/bout/boutmesh.cxx:123: Taking false branch

  ASSERT0(MYG >= 0);
  ^

include/bout/assert.hxx:29: expanded from macro 'ASSERT0'

  if (!(condition)) {                                                                    \
  ^

src/mesh/impls/bout/boutmesh.cxx:127: Field 'MZG' is >= 0

  ASSERT0(MZG >= 0);
          ^

src/mesh/impls/bout/boutmesh.cxx:127: Taking false branch

  ASSERT0(MZG >= 0);
  ^

include/bout/assert.hxx:29: expanded from macro 'ASSERT0'

  if (!(condition)) {                                                                    \
  ^

src/mesh/impls/bout/boutmesh.cxx:147: Assuming the condition is false

  if (options.isSet("NXPE") or options.isSet("NYPE")) {
      ^

src/mesh/impls/bout/boutmesh.cxx:147: Left side of '||' is false

  if (options.isSet("NXPE") or options.isSet("NYPE")) {
      ^

src/mesh/impls/bout/boutmesh.cxx:147: Assuming the condition is true

  if (options.isSet("NXPE") or options.isSet("NYPE")) {
      ^

src/mesh/impls/bout/boutmesh.cxx:147: Taking true branch

  if (options.isSet("NXPE") or options.isSet("NYPE")) {
  ^

src/mesh/impls/bout/boutmesh.cxx:168: Assuming the condition is false

  if (options.isSet("zperiod")) {
      ^

src/mesh/impls/bout/boutmesh.cxx:168: Taking false branch

  if (options.isSet("zperiod")) {
  ^

src/mesh/impls/bout/boutmesh.cxx:194: Assuming the condition is false

  if (!source->get(this, ShiftAngle, "ShiftAngle", LocalNx, getGlobalXIndex(0))) {
      ^

src/mesh/impls/bout/boutmesh.cxx:194: Taking false branch

  if (!source->get(this, ShiftAngle, "ShiftAngle", LocalNx, getGlobalXIndex(0))) {
  ^

src/mesh/impls/bout/boutmesh.cxx:198: Assuming field 'TwistShift' is false

  if (TwistShift) {
      ^

src/mesh/impls/bout/boutmesh.cxx:198: Taking false branch

  if (TwistShift) {
  ^

src/mesh/impls/bout/boutmesh.cxx:218: Assuming the condition is true

  if (possible_boundaries.empty()) {
      ^

src/mesh/impls/bout/boutmesh.cxx:218: Taking true branch

  if (possible_boundaries.empty()) {
  ^

src/mesh/impls/bout/boutmesh.cxx:227: Assuming the condition is false

  if (!boundary.empty()) {
      ^

src/mesh/impls/bout/boutmesh.cxx:227: Taking false branch

  if (!boundary.empty()) {
  ^

src/mesh/impls/bout/boutmesh.cxx:245: Assuming the condition is false

    int mybndry = static_cast<int>(!(iterateBndryLowerY().isDone()));
                                   ^

src/mesh/impls/bout/boutmesh.cxx:247: Call to virtual method 'BoutMesh::getXcomm' during construction bypasses virtual dispatch

    mpi->MPI_Allreduce(&mybndry, &allbndry, 1, MPI_INT, MPI_BOR, getXcomm(yend));
                                                                 ^

src/mesh/impls/bout/boutmesh.cxx Outdated
has_boundary_lower_y = static_cast<bool>(allbndry);
}
{
int mybndry = static_cast<int>(!(iterateBndryUpperY().isDone()));
Copy link
Contributor

@github-actions github-actions bot Nov 24, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

warning: Call to virtual method 'BoutMesh::iterateBndryUpperY' during construction bypasses virtual dispatch [clang-analyzer-optin.cplusplus.VirtualCall]

    int mybndry = static_cast<int>(!(iterateBndryUpperY().isDone()));
                                     ^
Additional context

src/mesh/impls/bout/boutmesh.cxx:56: Assuming the condition is false

  if (!options->isSet("symmetricGlobalY")) {
      ^

src/mesh/impls/bout/boutmesh.cxx:56: Taking false branch

  if (!options->isSet("symmetricGlobalY")) {
  ^

src/mesh/impls/bout/boutmesh.cxx:83: Assuming the condition is false

  if (Mesh::get(nx, "nx") != 0) {
      ^

src/mesh/impls/bout/boutmesh.cxx:83: Taking false branch

  if (Mesh::get(nx, "nx") != 0) {
  ^

src/mesh/impls/bout/boutmesh.cxx:87: Assuming the condition is false

  if (Mesh::get(ny, "ny") != 0) {
      ^

src/mesh/impls/bout/boutmesh.cxx:87: Taking false branch

  if (Mesh::get(ny, "ny") != 0) {
  ^

src/mesh/impls/bout/boutmesh.cxx:91: Assuming the condition is false

  if (Mesh::get(nz, "nz") != 0) {
      ^

src/mesh/impls/bout/boutmesh.cxx:91: Taking false branch

  if (Mesh::get(nz, "nz") != 0) {
  ^

src/mesh/impls/bout/boutmesh.cxx:114: Assuming the condition is false

  if (Mesh::get(MXG, "MXG") != 0) {
      ^

src/mesh/impls/bout/boutmesh.cxx:114: Taking false branch

  if (Mesh::get(MXG, "MXG") != 0) {
  ^

src/mesh/impls/bout/boutmesh.cxx:118: Assuming field 'MXG' is >= 0

  ASSERT0(MXG >= 0);
          ^

include/bout/assert.hxx:29: expanded from macro 'ASSERT0'

  if (!(condition)) {                                                                    \
        ^

src/mesh/impls/bout/boutmesh.cxx:118: Taking false branch

  ASSERT0(MXG >= 0);
  ^

include/bout/assert.hxx:29: expanded from macro 'ASSERT0'

  if (!(condition)) {                                                                    \
  ^

src/mesh/impls/bout/boutmesh.cxx:120: Assuming the condition is false

  if (Mesh::get(MYG, "MYG") != 0) {
      ^

src/mesh/impls/bout/boutmesh.cxx:120: Taking false branch

  if (Mesh::get(MYG, "MYG") != 0) {
  ^

src/mesh/impls/bout/boutmesh.cxx:123: Assuming field 'MYG' is >= 0

  ASSERT0(MYG >= 0);
          ^

include/bout/assert.hxx:29: expanded from macro 'ASSERT0'

  if (!(condition)) {                                                                    \
        ^

src/mesh/impls/bout/boutmesh.cxx:123: Taking false branch

  ASSERT0(MYG >= 0);
  ^

include/bout/assert.hxx:29: expanded from macro 'ASSERT0'

  if (!(condition)) {                                                                    \
  ^

src/mesh/impls/bout/boutmesh.cxx:127: Field 'MZG' is >= 0

  ASSERT0(MZG >= 0);
          ^

src/mesh/impls/bout/boutmesh.cxx:127: Taking false branch

  ASSERT0(MZG >= 0);
  ^

include/bout/assert.hxx:29: expanded from macro 'ASSERT0'

  if (!(condition)) {                                                                    \
  ^

src/mesh/impls/bout/boutmesh.cxx:147: Assuming the condition is false

  if (options.isSet("NXPE") or options.isSet("NYPE")) {
      ^

src/mesh/impls/bout/boutmesh.cxx:147: Left side of '||' is false

  if (options.isSet("NXPE") or options.isSet("NYPE")) {
      ^

src/mesh/impls/bout/boutmesh.cxx:147: Assuming the condition is true

  if (options.isSet("NXPE") or options.isSet("NYPE")) {
      ^

src/mesh/impls/bout/boutmesh.cxx:147: Taking true branch

  if (options.isSet("NXPE") or options.isSet("NYPE")) {
  ^

src/mesh/impls/bout/boutmesh.cxx:168: Assuming the condition is false

  if (options.isSet("zperiod")) {
      ^

src/mesh/impls/bout/boutmesh.cxx:168: Taking false branch

  if (options.isSet("zperiod")) {
  ^

src/mesh/impls/bout/boutmesh.cxx:194: Assuming the condition is false

  if (!source->get(this, ShiftAngle, "ShiftAngle", LocalNx, getGlobalXIndex(0))) {
      ^

src/mesh/impls/bout/boutmesh.cxx:194: Taking false branch

  if (!source->get(this, ShiftAngle, "ShiftAngle", LocalNx, getGlobalXIndex(0))) {
  ^

src/mesh/impls/bout/boutmesh.cxx:198: Assuming field 'TwistShift' is false

  if (TwistShift) {
      ^

src/mesh/impls/bout/boutmesh.cxx:198: Taking false branch

  if (TwistShift) {
  ^

src/mesh/impls/bout/boutmesh.cxx:218: Assuming the condition is true

  if (possible_boundaries.empty()) {
      ^

src/mesh/impls/bout/boutmesh.cxx:218: Taking true branch

  if (possible_boundaries.empty()) {
  ^

src/mesh/impls/bout/boutmesh.cxx:227: Assuming the condition is false

  if (!boundary.empty()) {
      ^

src/mesh/impls/bout/boutmesh.cxx:227: Taking false branch

  if (!boundary.empty()) {
  ^

src/mesh/impls/bout/boutmesh.cxx:245: Assuming the condition is false

    int mybndry = static_cast<int>(!(iterateBndryLowerY().isDone()));
                                   ^

src/mesh/impls/bout/boutmesh.cxx:251: Call to virtual method 'BoutMesh::iterateBndryUpperY' during construction bypasses virtual dispatch

    int mybndry = static_cast<int>(!(iterateBndryUpperY().isDone()));
                                     ^

src/mesh/impls/bout/boutmesh.cxx Outdated
{
int mybndry = static_cast<int>(!(iterateBndryUpperY().isDone()));
int allbndry = 0;
mpi->MPI_Allreduce(&mybndry, &allbndry, 1, MPI_INT, MPI_BOR, getXcomm(ystart));
Copy link
Contributor

@github-actions github-actions bot Nov 24, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

warning: Call to virtual method 'BoutMesh::getXcomm' during construction bypasses virtual dispatch [clang-analyzer-optin.cplusplus.VirtualCall]

    mpi->MPI_Allreduce(&mybndry, &allbndry, 1, MPI_INT, MPI_BOR, getXcomm(ystart));
                                                                 ^
Additional context

src/mesh/impls/bout/boutmesh.cxx:56: Assuming the condition is false

  if (!options->isSet("symmetricGlobalY")) {
      ^

src/mesh/impls/bout/boutmesh.cxx:56: Taking false branch

  if (!options->isSet("symmetricGlobalY")) {
  ^

src/mesh/impls/bout/boutmesh.cxx:83: Assuming the condition is false

  if (Mesh::get(nx, "nx") != 0) {
      ^

src/mesh/impls/bout/boutmesh.cxx:83: Taking false branch

  if (Mesh::get(nx, "nx") != 0) {
  ^

src/mesh/impls/bout/boutmesh.cxx:87: Assuming the condition is false

  if (Mesh::get(ny, "ny") != 0) {
      ^

src/mesh/impls/bout/boutmesh.cxx:87: Taking false branch

  if (Mesh::get(ny, "ny") != 0) {
  ^

src/mesh/impls/bout/boutmesh.cxx:91: Assuming the condition is false

  if (Mesh::get(nz, "nz") != 0) {
      ^

src/mesh/impls/bout/boutmesh.cxx:91: Taking false branch

  if (Mesh::get(nz, "nz") != 0) {
  ^

src/mesh/impls/bout/boutmesh.cxx:114: Assuming the condition is false

  if (Mesh::get(MXG, "MXG") != 0) {
      ^

src/mesh/impls/bout/boutmesh.cxx:114: Taking false branch

  if (Mesh::get(MXG, "MXG") != 0) {
  ^

src/mesh/impls/bout/boutmesh.cxx:118: Assuming field 'MXG' is >= 0

  ASSERT0(MXG >= 0);
          ^

include/bout/assert.hxx:29: expanded from macro 'ASSERT0'

  if (!(condition)) {                                                                    \
        ^

src/mesh/impls/bout/boutmesh.cxx:118: Taking false branch

  ASSERT0(MXG >= 0);
  ^

include/bout/assert.hxx:29: expanded from macro 'ASSERT0'

  if (!(condition)) {                                                                    \
  ^

src/mesh/impls/bout/boutmesh.cxx:120: Assuming the condition is false

  if (Mesh::get(MYG, "MYG") != 0) {
      ^

src/mesh/impls/bout/boutmesh.cxx:120: Taking false branch

  if (Mesh::get(MYG, "MYG") != 0) {
  ^

src/mesh/impls/bout/boutmesh.cxx:123: Assuming field 'MYG' is >= 0

  ASSERT0(MYG >= 0);
          ^

include/bout/assert.hxx:29: expanded from macro 'ASSERT0'

  if (!(condition)) {                                                                    \
        ^

src/mesh/impls/bout/boutmesh.cxx:123: Taking false branch

  ASSERT0(MYG >= 0);
  ^

include/bout/assert.hxx:29: expanded from macro 'ASSERT0'

  if (!(condition)) {                                                                    \
  ^

src/mesh/impls/bout/boutmesh.cxx:127: Field 'MZG' is >= 0

  ASSERT0(MZG >= 0);
          ^

src/mesh/impls/bout/boutmesh.cxx:127: Taking false branch

  ASSERT0(MZG >= 0);
  ^

include/bout/assert.hxx:29: expanded from macro 'ASSERT0'

  if (!(condition)) {                                                                    \
  ^

src/mesh/impls/bout/boutmesh.cxx:147: Assuming the condition is false

  if (options.isSet("NXPE") or options.isSet("NYPE")) {
      ^

src/mesh/impls/bout/boutmesh.cxx:147: Left side of '||' is false

  if (options.isSet("NXPE") or options.isSet("NYPE")) {
      ^

src/mesh/impls/bout/boutmesh.cxx:147: Assuming the condition is true

  if (options.isSet("NXPE") or options.isSet("NYPE")) {
      ^

src/mesh/impls/bout/boutmesh.cxx:147: Taking true branch

  if (options.isSet("NXPE") or options.isSet("NYPE")) {
  ^

src/mesh/impls/bout/boutmesh.cxx:168: Assuming the condition is false

  if (options.isSet("zperiod")) {
      ^

src/mesh/impls/bout/boutmesh.cxx:168: Taking false branch

  if (options.isSet("zperiod")) {
  ^

src/mesh/impls/bout/boutmesh.cxx:194: Assuming the condition is false

  if (!source->get(this, ShiftAngle, "ShiftAngle", LocalNx, getGlobalXIndex(0))) {
      ^

src/mesh/impls/bout/boutmesh.cxx:194: Taking false branch

  if (!source->get(this, ShiftAngle, "ShiftAngle", LocalNx, getGlobalXIndex(0))) {
  ^

src/mesh/impls/bout/boutmesh.cxx:198: Assuming field 'TwistShift' is false

  if (TwistShift) {
      ^

src/mesh/impls/bout/boutmesh.cxx:198: Taking false branch

  if (TwistShift) {
  ^

src/mesh/impls/bout/boutmesh.cxx:218: Assuming the condition is true

  if (possible_boundaries.empty()) {
      ^

src/mesh/impls/bout/boutmesh.cxx:218: Taking true branch

  if (possible_boundaries.empty()) {
  ^

src/mesh/impls/bout/boutmesh.cxx:227: Assuming the condition is false

  if (!boundary.empty()) {
      ^

src/mesh/impls/bout/boutmesh.cxx:227: Taking false branch

  if (!boundary.empty()) {
  ^

src/mesh/impls/bout/boutmesh.cxx:245: Assuming the condition is false

    int mybndry = static_cast<int>(!(iterateBndryLowerY().isDone()));
                                   ^

src/mesh/impls/bout/boutmesh.cxx:251: Assuming the condition is false

    int mybndry = static_cast<int>(!(iterateBndryUpperY().isDone()));
                                   ^

src/mesh/impls/bout/boutmesh.cxx:253: Call to virtual method 'BoutMesh::getXcomm' during construction bypasses virtual dispatch

    mpi->MPI_Allreduce(&mybndry, &allbndry, 1, MPI_INT, MPI_BOR, getXcomm(ystart));
                                                                 ^

src/mesh/impls/bout/boutmesh.cxx
mpi->MPI_Irecv(buffer, size, PVEC_REAL_MPI_TYPE, proc, tag,
BoutComm::get(), ch->request);
mpi->MPI_Irecv(buffer, size, PVEC_REAL_MPI_TYPE, proc, tag, BoutComm::get(),
ch->request);
Copy link
Contributor

@github-actions github-actions bot Nov 24, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

warning: do not implicitly decay an array into a pointer; consider using gsl::array_view or an explicit cast instead [cppcoreguidelines-pro-bounds-array-to-pointer-decay]

                 ch->request);
                 ^

src/mesh/impls/bout/boutmesh.cxx
mpi->MPI_Irecv(buffer, size, PVEC_REAL_MPI_TYPE, proc, tag,
BoutComm::get(), ch->request);
mpi->MPI_Irecv(buffer, size, PVEC_REAL_MPI_TYPE, proc, tag, BoutComm::get(),
ch->request);
Copy link
Contributor

@github-actions github-actions bot Nov 24, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

warning: do not implicitly decay an array into a pointer; consider using gsl::array_view or an explicit cast instead [cppcoreguidelines-pro-bounds-array-to-pointer-decay]

                 ch->request);
                 ^

@ZedThree
Copy link
Member Author

ZedThree commented Nov 24, 2025 •
edited
Loading

Ah, clang-tidy has spotted a bunch of virtual functions that will need qualifying with BoutMesh::

Yeah, and some tests fail, so this trivial change is definitely not enough. Shame

@ZedThree
Copy link
Member Author

ZedThree commented Dec 2, 2025

Ok the more fundamental problem is that we may need to read some quite important variables (nx, for instance) from Options, which involves parsing a string, which involves creating the FieldFactory singleton, which requires a Mesh* -- and we have no way of passing a Mesh* through Options to get to the FieldFactory. Actually, we have no way of creating the FieldFactory singleton on anything but bout::globals::mesh either.

I suppose we could have something like FieldFactory::setSingletonMesh() to set this in the BoutMesh constructor, but that does feel a little gross

github-actions[bot]
github-actions bot reviewed Dec 2, 2025
View reviewed changes
Copy link
Contributor

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

clang-tidy made some suggestions

src/mesh/impls/bout/boutmesh.cxx
NZPE = 1;

output_info << _("\tGuard cells (x,y,z): ") << MXG << ", " << MYG << ", " << MZG
<< std::endl;
Copy link
Contributor

@github-actions github-actions bot Dec 2, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

warning: no header providing "std::endl" is directly included [misc-include-cleaner]

src/mesh/impls/bout/boutmesh.cxx:46:

- #include <set>
+ #include <ostream>
+ #include <set>

src/mesh/impls/bout/boutmesh.cxx

ShiftAngle.resize(LocalNx);

if (!source->get(this, ShiftAngle, "ShiftAngle", LocalNx, getGlobalXIndex(0))) {
Copy link
Contributor

@github-actions github-actions bot Dec 2, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

warning: Call to virtual method 'BoutMesh::getGlobalXIndex' during construction bypasses virtual dispatch [clang-analyzer-optin.cplusplus.VirtualCall]

  if (!source->get(this, ShiftAngle, "ShiftAngle", LocalNx, getGlobalXIndex(0))) {
                                                            ^
Additional context

src/mesh/impls/bout/boutmesh.cxx:57: Assuming the condition is false

  if (!options->isSet("symmetricGlobalY")) {
      ^

src/mesh/impls/bout/boutmesh.cxx:57: Taking false branch

  if (!options->isSet("symmetricGlobalY")) {
  ^

src/mesh/impls/bout/boutmesh.cxx:85: Assuming the condition is false

  if (Mesh::get(nx, "nx") != 0) {
      ^

src/mesh/impls/bout/boutmesh.cxx:85: Taking false branch

  if (Mesh::get(nx, "nx") != 0) {
  ^

src/mesh/impls/bout/boutmesh.cxx:89: Assuming the condition is false

  if (Mesh::get(ny, "ny") != 0) {
      ^

src/mesh/impls/bout/boutmesh.cxx:89: Taking false branch

  if (Mesh::get(ny, "ny") != 0) {
  ^

src/mesh/impls/bout/boutmesh.cxx:93: Assuming the condition is false

  if (Mesh::get(nz, "nz") != 0) {
      ^

src/mesh/impls/bout/boutmesh.cxx:93: Taking false branch

  if (Mesh::get(nz, "nz") != 0) {
  ^

src/mesh/impls/bout/boutmesh.cxx:116: Assuming the condition is false

  if (Mesh::get(MXG, "MXG") != 0) {
      ^

src/mesh/impls/bout/boutmesh.cxx:116: Taking false branch

  if (Mesh::get(MXG, "MXG") != 0) {
  ^

src/mesh/impls/bout/boutmesh.cxx:120: Assuming field 'MXG' is >= 0

  ASSERT0(MXG >= 0);
          ^

include/bout/assert.hxx:29: expanded from macro 'ASSERT0'

  if (!(condition)) {                                                                    \
        ^

src/mesh/impls/bout/boutmesh.cxx:120: Taking false branch

  ASSERT0(MXG >= 0);
  ^

include/bout/assert.hxx:29: expanded from macro 'ASSERT0'

  if (!(condition)) {                                                                    \
  ^

src/mesh/impls/bout/boutmesh.cxx:122: Assuming the condition is false

  if (Mesh::get(MYG, "MYG") != 0) {
      ^

src/mesh/impls/bout/boutmesh.cxx:122: Taking false branch

  if (Mesh::get(MYG, "MYG") != 0) {
  ^

src/mesh/impls/bout/boutmesh.cxx:125: Assuming field 'MYG' is >= 0

  ASSERT0(MYG >= 0);
          ^

include/bout/assert.hxx:29: expanded from macro 'ASSERT0'

  if (!(condition)) {                                                                    \
        ^

src/mesh/impls/bout/boutmesh.cxx:125: Taking false branch

  ASSERT0(MYG >= 0);
  ^

include/bout/assert.hxx:29: expanded from macro 'ASSERT0'

  if (!(condition)) {                                                                    \
  ^

src/mesh/impls/bout/boutmesh.cxx:129: Field 'MZG' is >= 0

  ASSERT0(MZG >= 0);
          ^

src/mesh/impls/bout/boutmesh.cxx:129: Taking false branch

  ASSERT0(MZG >= 0);
  ^

include/bout/assert.hxx:29: expanded from macro 'ASSERT0'

  if (!(condition)) {                                                                    \
  ^

src/mesh/impls/bout/boutmesh.cxx:149: Assuming the condition is false

  if (options.isSet("NXPE") or options.isSet("NYPE")) {
      ^

src/mesh/impls/bout/boutmesh.cxx:149: Left side of '||' is false

  if (options.isSet("NXPE") or options.isSet("NYPE")) {
      ^

src/mesh/impls/bout/boutmesh.cxx:149: Assuming the condition is true

  if (options.isSet("NXPE") or options.isSet("NYPE")) {
      ^

src/mesh/impls/bout/boutmesh.cxx:149: Taking true branch

  if (options.isSet("NXPE") or options.isSet("NYPE")) {
  ^

src/mesh/impls/bout/boutmesh.cxx:170: Assuming the condition is true

  if (options.isSet("zperiod")) {
      ^

src/mesh/impls/bout/boutmesh.cxx:170: Taking true branch

  if (options.isSet("zperiod")) {
  ^

src/mesh/impls/bout/boutmesh.cxx:196: Call to virtual method 'BoutMesh::getGlobalXIndex' during construction bypasses virtual dispatch

  if (!source->get(this, ShiftAngle, "ShiftAngle", LocalNx, getGlobalXIndex(0))) {
                                                            ^

@bendudson
Copy link
Contributor

bendudson commented Dec 2, 2025

Ok the more fundamental problem is that we may need to read some quite important variables (nx, for instance) from Options, which involves parsing a string, which involves creating the FieldFactory singleton, which requires a Mesh* -- and we have no way of passing a Mesh* through Options to get to the FieldFactory. Actually, we have no way of creating the FieldFactory singleton on anything but bout::globals::mesh either.

I suppose we could have something like FieldFactory::setSingletonMesh() to set this in the BoutMesh constructor, but that does feel a little gross

Perhaps we could remove the Mesh* argument to the FieldFactory constructor. It sets a default Mesh*, overridden by optional Mesh* arguments to create2D and create3D methods. Those arguments would now be required.

@ZedThree
Copy link
Member Author

ZedThree commented Dec 2, 2025 •
edited
Loading

Perhaps we could remove the Mesh* argument to the FieldFactory constructor. It sets a default Mesh*, overridden by optional Mesh* arguments to create2D and create3D methods. Those arguments would now be required.

That could work, I think we'd also need to add a Mesh* argument to Options::as (at least for the field versions?)

EDIT: I have added the static FieldFactory::setMesh(Mesh*) here and it seems to work -- the tests all pass at least! I think this experiment mostly highlights where we have circular dependencies between classes

@ZedThree ZedThree marked this pull request as draft January 7, 2026 09:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@github-actions github-actions[bot] github-actions[bot] left review comments

At least 1 approving review is required to merge this pull request.

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@ZedThree @bendudson