Skip to content

Handle PETSc signature change for FormFunctionForColoring#3116

Closed
tomc271 wants to merge 6 commits intonextfrom
fix-FormFunctionForColoring-issue
Closed

Handle PETSc signature change for FormFunctionForColoring#3116
tomc271 wants to merge 6 commits intonextfrom
fix-FormFunctionForColoring-issue

Conversation

@tomc271
Copy link
Copy Markdown
Collaborator

@tomc271 tomc271 commented Jun 4, 2025

Use a wrapper function to handle FormFunctionForColoring signature mismatch, as using reinterpret_cast no longer works.

Expected signature is PetscErrorCode (*)()
but is PetscErrorCode (*)(SNES, Vec, Vec, void*) in newer PETSc versions (on main branch, but no releases yet).

For the newer version the current solver instance must be passed as the 'cxt' argument,
so store 'this' instance in a global variable (before calling MatFDColoringSetFunction).

Have to do it this way because the function passed as the (second) argument to MatFDColoringSetFunction must be a free function, so cannot make the function wrapper an instance method and pass 'this' from there.

…smatch

Using reinterpret_cast no longer works.

Expected signature is PetscErrorCode (*)()
but is PetscErrorCode (*)(SNES, Vec, Vec, void*) on main branch.

For the newer version the current solver instance must be passed as the 'cxt' argument,
so store 'this' instance in a global variable (before calling MatFDColoringSetFunction).

Have to do it this way because the function passed as the (second) argument to MatFDColoringSetFunction must be a free function,
so cannot make the function wrapper an instance method and pass 'this' from there.
Use a wrapper around solver_f for passing as the second argument to MatFDColoringSetFunction.
@tomc271 tomc271 requested a review from ZedThree June 4, 2025 18:28
Copy link
Copy Markdown
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

}

static PetscErrorCode imexbdf2PCapply(PC pc, Vec x, Vec y) {
// Global context to store IMEXBDF2 instance
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

warning: variable 'imexbdf2_ctx' declared 'static', move to anonymous namespace instead [misc-use-anonymous-namespace]

static void* imexbdf2_ctx = nullptr;
             ^

}

static PetscErrorCode imexbdf2PCapply(PC pc, Vec x, Vec y) {
// Global context to store IMEXBDF2 instance
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

warning: variable 'imexbdf2_ctx' is non-const and globally accessible, consider making it const [cppcoreguidelines-avoid-non-const-global-variables]

static void* imexbdf2_ctx = nullptr;
             ^

}

static PetscErrorCode imexbdf2PCapply(PC pc, Vec x, Vec y) {
// Global context to store IMEXBDF2 instance
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

warning: variable 'imexbdf2_ctx' provides global access to a non-const object; consider making the pointed-to data 'const' [cppcoreguidelines-avoid-non-const-global-variables]

static void* imexbdf2_ctx = nullptr;
             ^

Comment thread src/solver/impls/imex-bdf2/imex-bdf2.cxx
Comment thread src/solver/impls/imex-bdf2/imex-bdf2.cxx
}

// Global context to store SNESSolver instance
static void* snes_ctx = nullptr;
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

warning: variable 'snes_ctx' provides global access to a non-const object; consider making the pointed-to data 'const' [cppcoreguidelines-avoid-non-const-global-variables]

static void* snes_ctx = nullptr;
             ^

Comment thread src/solver/impls/snes/snes.cxx
Comment thread src/solver/impls/snes/snes.cxx
}
#else
// Wrapper for PETSc < 3.20 (signature: PetscErrorCode (*)(void))
static PetscErrorCode FormFunctionForColoringWrapper() {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

warning: function 'FormFunctionForColoringWrapper' declared 'static', move to anonymous namespace instead [misc-use-anonymous-namespace]

static PetscErrorCode FormFunctionForColoringWrapper() {
                      ^

Comment thread src/solver/impls/snes/snes.cxx
@ZedThree
Copy link
Copy Markdown
Member

ZedThree commented Jun 5, 2025

Thanks @tomc271, unfortunately this approach won't work -- although MatFDColoringSetFunction takes a function PetscErrorCode (*f)(void), it's actually lying about the signature. FormFunctionForColoring already has the correct signature for when it's called, it's just that MatFDColoringSetFunction expects it to be passed with the wrong signature, hence the requirement for reinterpret_cast. Yes this is disgusting and wrong, no we can't do much about it.

@tomc271
Copy link
Copy Markdown
Collaborator Author

tomc271 commented Jun 5, 2025

Ok. It got all the tests to pass, so I guess there is no test covering that scenario.

@ZedThree
Copy link
Copy Markdown
Member

ZedThree commented Jun 5, 2025

Hmm, I did think we had tests on this, maybe they're not on by default

@ZedThree ZedThree closed this Jun 11, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants