Fix reference to local variable.

[1uc]

During CI we see the following compiler warning:

/root/nrn/external/nmodl/ext/pybind11/include/pybind11/cast.h:1054:35: note: declared here
 1054 |     return cast_op<T>(load_type<T>(handle));
      |                       ~~~~~~~~~~~~^~~~~~~~

e.g. https://app.circleci.com/pipelines/github/neuronsimulator/nrn/5301/workflows/c43f0a31-36ec-4e95-93df-7df650db01d4/jobs/2887?invite=true#step-102-127220_85

If we track the definition of cast_op and load_type we find:

template <typename T>
typename make_caster<T>::template cast_op_type<T> cast_op(make_caster<T> &caster) {
    return caster.operator typename make_caster<T>::template cast_op_type<T>();
}

and

template <typename T>
make_caster<T> load_type(const handle &handle) {
    make_caster<T> conv;
    load_type(conv, handle);
    return conv;
}

The suspicion is that make_caster<T> will strip the & and store a copy

template <typename type>
using make_caster = type_caster<intrinsic_t<type>>;

However, then the returned reference does point to a local variable.

We fix this by returning std::shared_ptr by value.

[bbpbuildbot]

Logfiles from GitLab pipeline #149850 (:no_entry:) have been uploaded here!

Status and direct links:

• ✅ spack_setup
• ✅ build:intel
• ✅ build:nvhpc
• ✅ test:intel
• ✅ test:nvhpc

[codecov]

Codecov Report

Patch coverage: 41.66% and project coverage change: -0.03% ⚠️

Comparison is base (5339400) 69.98% compared to head (cc0d6ef) 69.96%.
Report is 2 commits behind head on master.

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #1060      +/-   ##
==========================================
- Coverage   69.98%   69.96%   -0.03%     
==========================================
  Files         188      188              
  Lines       25659    25661       +2     
==========================================
- Hits        17957    17953       -4     
- Misses       7702     7708       +6     

Files Changed	Coverage Δ
src/language/templates/ast/ast.cpp	48.67% <0.00%> (-0.09%)	⬇️
src/language/templates/ast/ast.hpp	50.00% <ø> (ø)
src/language/templates/pybind/pyast.hpp	0.00% <0.00%> (ø)
src/visitors/sympy_replace_solutions_visitor.hpp	100.00% <ø> (ø)
src/visitors/sympy_replace_solutions_visitor.cpp	90.30% <33.33%> (+0.04%)	⬆️
src/codegen/codegen_cpp_visitor.cpp	85.82% <100.00%> (ø)
src/visitors/global_var_visitor.cpp	95.83% <100.00%> (ø)
src/visitors/neuron_solve_visitor.cpp	100.00% <100.00%> (ø)

... and 2 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[1uc]

These functions is covered by tests:

  void SympyReplaceSolutionsVisitor::visit_diff_eq_expression(ast::DiffEqExpression& node)
  void SympyReplaceSolutionsVisitor::visit_non_lin_equation(ast::NonLinEquation& node)

[1uc]

It's slightly more complicated. Pybind11 supports returning const references. I believe those usually take the first branch in:

#define PYBIND11_OVERRIDE_IMPL(ret_type, cname, name, ...)                                        \
    do {                                                                                          \
        pybind11::gil_scoped_acquire gil;                                                         \
        pybind11::function override                                                               \
            = pybind11::get_override(static_cast<const cname *>(this), name);                     \
        if (override) {                                                                           \
            auto o = override(__VA_ARGS__);                                                       \
            if (pybind11::detail::cast_is_temporary_value_reference<ret_type>::value) {           \
                static pybind11::detail::override_caster_t<ret_type> caster;                      \
                return pybind11::detail::cast_ref<ret_type>(std::move(o), caster);                \
            }                                                                                     \
            return pybind11::detail::cast_safe<ret_type>(std::move(o));                           \
        }                                                                                         \
    } while (false)

https://github.com/pybind/pybind11/blob/master/include/pybind11/pybind11.h#L2829

We can check that:

pybind11::detail::cast_is_temporary_value_reference<const std::string&>::value == true

but

pybind11::detail::cast_is_temporary_value_reference<const std::shared_ptr<T>&>::value == false

which explains why the warning point to code that's called via:

            return pybind11::detail::cast_safe<ret_type>(std::move(o));                           \

rather than

                return pybind11::detail::cast_ref<ret_type>(std::move(o), caster);                \

(I've tried adding if constexpr to make sure the other branch isn't considered.)

Given the documented limitations with returning const-references, see
https://pybind11.readthedocs.io/en/stable/advanced/classes.html#overriding-virtual-functions-in-python
(see Notes at the end of that paragraph), I'd still propose to avoid the issue by returning by value.

[pramodk]

LGTM.

Just for clarity: PyAst::get_statement_block() is not a culprit here?

[1uc]

Sorry, somehow only a "fixup" commit made it to github (bad rebase or something). The first commit is the important one (that was missing).