neuronsimulator / nrn

NEURON Simulator
http://nrn.readthedocs.io
Other
395 stars 116 forks source link

Distinguish `OpaquePointer`s from `double*`. #2940

Closed 1uc closed 2 months ago

1uc commented 3 months ago

Inside VERBATIM blocks of a MOD file, pointers can be made to point to any type, e.g. void*. The generic_data_handle knows this, and will refuse any attempts to convert to double*.

From Python there's certain pointers are visible, even if they don't point at a double. They were unconditionally converted to a data_handle<double>, i.e. a double*. If the generic_data_handle didn't contain a double* the application would crash.

This commit proposes to return an OpaquePointer object instead. There's nothing one can do with an OpaquePointer, but it helps in things like psection.

1uc commented 3 months ago

The original issue reporting an issue is #2783.

azure-pipelines[bot] commented 3 months ago

✔️ 18b67f26a2ad19e69c2e43ae7724863fae7c1d7b -> Azure artifacts URL

codecov[bot] commented 3 months ago

Codecov Report

Attention: Patch coverage is 90.58824% with 8 lines in your changes missing coverage. Please review.

Project coverage is 67.29%. Comparing base (c2b939c) to head (ad7c2fd). Report is 2 commits behind head on master.

Files Patch % Lines
src/nrnpython/nrnpy_nrn.cpp 89.33% 8 Missing :warning:
Additional details and impacted files ```diff @@ Coverage Diff @@ ## master #2940 +/- ## ========================================== + Coverage 67.26% 67.29% +0.03% ========================================== Files 571 571 Lines 104876 104920 +44 ========================================== + Hits 70542 70610 +68 + Misses 34334 34310 -24 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

bbpbuildbot commented 3 months ago

Logfiles from GitLab pipeline #218812 (:white_check_mark:) have been uploaded here!

Status and direct links:

azure-pipelines[bot] commented 3 months ago

✔️ a503a4eaf9799683ac5dc6401bbcbef8c080d4f1 -> Azure artifacts URL

bbpbuildbot commented 3 months ago

Logfiles from GitLab pipeline #219157 (:white_check_mark:) have been uploaded here!

Status and direct links:

1uc commented 3 months ago

I've added tests for things like:

s.mech.ptr = 42.0
s.mech._ref_ptr = some_other_ref

in both variations, i.e. either s.mech.ptr or s.ptr_mech.

There's checks that it returns an OpaquePointer when appropriate. It also checks that we can't assign to a pointer that doesn't point to anything; and that we can't mix types, e.g. string to doubles or doubles to pointers.

The tests are all in Python and no new tests were added for HOC.

azure-pipelines[bot] commented 3 months ago

✔️ 4f6744ef7a9f3737e925c89a5ee3542162eaa0bf -> Azure artifacts URL

azure-pipelines[bot] commented 3 months ago

✔️ 150e6c516af72778a814175bfcea7cc93a17c563 -> Azure artifacts URL

bbpbuildbot commented 3 months ago

Logfiles from GitLab pipeline #219227 (:white_check_mark:) have been uploaded here!

Status and direct links:

bbpbuildbot commented 2 months ago

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

Status and direct links:

azure-pipelines[bot] commented 2 months ago

✔️ 322ee69df7a3dc01e4f1720a80160c88f9817e3f -> Azure artifacts URL

sonarcloud[bot] commented 2 months ago

Quality Gate Passed Quality Gate passed

Issues
12 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarCloud

bbpbuildbot commented 2 months ago

Logfiles from GitLab pipeline #220713 (:white_check_mark:) have been uploaded here!

Status and direct links:

azure-pipelines[bot] commented 2 months ago

✔️ ad7c2fd3af041fb9d66a867b32f62a611e73e956 -> Azure artifacts URL

pramodk commented 2 months ago

From our internal discussion, we said this is good to go!

@nrnhines: if you see any corner cases, let us know!