Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Example of how Datum works with generic_data_handle. #2466

Closed
wants to merge 3 commits into from

Conversation

1uc
Copy link
Collaborator

@1uc 1uc commented Aug 18, 2023

This commit adds an example (by means of unit-tests) that demonstrates the behaviour we see #2458. It involves understanding how raw pointers and pointers to values inside a soa are function. It further demonstrates the difference of generic_data_handle::get and generic_data_handle::literal_value.

This commit adds an example (by means of unit-tests) that demonstrates
the behaviour we see #2458. It involves understanding how raw pointers
and pointers to values inside a `soa` are function. It further
demonstrates the difference of `generic_data_handle::get` and
`generic_data_handle::literal_value`.
@bbpbuildbot

This comment has been minimized.

@azure-pipelines
Copy link

✔️ eaef32c -> Azure artifacts URL

@codecov
Copy link

codecov bot commented Aug 18, 2023

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 60.43%. Comparing base (efc28fe) to head (d18ddae).
Report is 428 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #2466      +/-   ##
==========================================
+ Coverage   60.41%   60.43%   +0.02%     
==========================================
  Files         626      627       +1     
  Lines      120812   120849      +37     
==========================================
+ Hits        72990    73039      +49     
+ Misses      47822    47810      -12     

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

@1uc 1uc marked this pull request as ready for review August 18, 2023 18:17
@bbpbuildbot

This comment has been minimized.

@azure-pipelines
Copy link

✔️ 3df0376 -> Azure artifacts URL

@azure-pipelines
Copy link

✔️ 4f37e6e -> Azure artifacts URL

@bbpbuildbot

This comment has been minimized.

src/nrnoc/hh.mod Outdated Show resolved Hide resolved
src/nrnoc/hh.mod Outdated Show resolved Hide resolved
// We set up everything such that the `soa` containers have two voltages.
// We'll now study what happens to a Datum that refers to one of these. In
// generated code this datum is called `_ppval[0]`. The
Datum* _ppval = (Datum*) malloc(sizeof(Datum));
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
Datum* _ppval = (Datum*) malloc(sizeof(Datum));
auto* const _ppval = new Datum{};

is the minimum change needed here, so that the constructor is called. Otherwise this is treating uninitialised memory as a Datum object.
I don't see why it needs to be on the heap, though.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It can be an std::vector. It doesn't need to be dynamically allocated. The point is to keep the similarity to the generated code. The name is _ppval[0]. Hence we need some things that gives us an operator[].

test/unit_tests/container/generic_data_handle.cpp Outdated Show resolved Hide resolved
Copy link
Collaborator

Choose a reason for hiding this comment

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

Perhaps we should move

TEST_CASE("generic_data_handle", "[Neuron][data_structures][generic_data_handle]") {

from node.cpp into here.

@1uc
Copy link
Collaborator Author

1uc commented Aug 23, 2023

This is just code to check that my understanding of the issue faced in #2458. I'm not sure we want to merge it at all.

Copy link
Member

@nrnhines nrnhines left a comment

Choose a reason for hiding this comment

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

Other than the accidental commit of hh.mod, this looks ready to merge.
However, given #2458 , I don't know if we'll change get<double*> to raise an error or add an alternative method that does raise an error or something else.

src/nrnoc/hh.mod Outdated Show resolved Hide resolved
@azure-pipelines
Copy link

✔️ 01a198e -> Azure artifacts URL

@azure-pipelines
Copy link

✔️ d18ddae -> Azure artifacts URL

@pramodk
Copy link
Member

pramodk commented Sep 4, 2023

@1uc : for merge, this should be rebased on master?

@1uc
Copy link
Collaborator Author

1uc commented Sep 5, 2023

I'm not sure we'd want to merge this. These tests aren't new. Some of them are dumb and demonstrate an obvious point (to the point of being confusing without the context).

The reason for this code is: In #2458 we were trying to figure out how these two macros would behave:

#define _p_c _ppvar[0].literal_value<void*>()
#define _p_c  _ppvar[0].get<double*>()

Since I was doing a lot of guessing, I needed to write code to check my understanding of the data structures. In order to share these examples, written in the form of tests because a) that's convenient way to get something to compile; and b) useful to assert various statements, I created this PR.

The conclusion is that neither is quite right:

  • literal_value fails when it happens to be modern.
  • get fails if one want to assign to the pointer.

Additionally,

  • For #define _c *_ppvar[0].get<double*>() there's the fear it could dereference a nullptr. Which is a user bug, but it doesn't manifest in a friendly manner.

@pramodk
Copy link
Member

pramodk commented Sep 5, 2023

Ok, I will let you / @nrnhines / @iomaganaris to decide on the usefulness. (Even if they are are not tests but useful examples for a new person, wondering if they should go somewhere.)

@pramodk pramodk marked this pull request as draft October 12, 2023 20:55
@1uc 1uc closed this Nov 26, 2024
@1uc 1uc deleted the 1uc/example-pointer branch November 26, 2024 16:14
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.

5 participants