Skip to content
This repository has been archived by the owner on Mar 20, 2023. It is now read-only.

Minor fixes #378

Merged
merged 6 commits into from
Aug 18, 2020
Merged

Minor fixes #378

merged 6 commits into from
Aug 18, 2020

Conversation

nrnhines
Copy link
Collaborator

I separated these into small independent commits. Especially solicit your opinion on not implementing assignment and copy constructors. Also, if those are useful to have, is it a good idea to specifically have delete correspond to new`` and freecorespond tomalloc```.

@pramodk
Copy link
Collaborator

pramodk commented Aug 17, 2020

@nrnhines : assignment is being used below:

/jenkins/07/workspace/oss.coreneuron.pipeline/tests/unit/interleave_info/check_constructors.cpp(70): error: function "coreneuron::InterleaveInfo::operator=(const coreneuron::InterleaveInfo &)" (declared at line 17 of "/jenkins/07/workspace/oss.coreneuron.pipeline/coreneuron/permute/cellorder.hpp") cannot be referenced -- it is a deleted function
      info3 = info1;

@alexsavulescu
Copy link
Contributor

alexsavulescu commented Aug 17, 2020

Especially solicit your opinion on not implementing assignment and copy constructors.

It depends on usage. What would be the case to copy or assign InterleaveInfo data? If InterleaveInfo is doing only resource management, then we wouldn't normally need them.

Also, if those are useful to have, is it a good idea to specifically have delete correspond to new`` and freecorespond tomalloc```.

new/malloc and delete/free can be interleaved, but the deallocation shall always match the allocation otherwise things can get problematic.

@nrnhines
Copy link
Collaborator Author

assignment is being used

Ok. So the choice is to revert 7dc3acc or to change the test. I don't have much of an opinion either way. Which would you prefer?

No need to have virtual

I don't expect to subclass in the future... Just a habit. Will get rid of it.

@pramodk pramodk closed this Aug 17, 2020
@pramodk pramodk reopened this Aug 17, 2020
@pramodk
Copy link
Collaborator

pramodk commented Aug 17, 2020

So the choice is to revert 7dc3acc or to change the test. I don't have much of an opinion either way. Which would you prefer?

yeah, reverting is fine!

@nrnhines
Copy link
Collaborator Author

Put back the assignment and copy constructors. Is it still worthwhile to make the destructor non-virtual?

@pramodk
Copy link
Collaborator

pramodk commented Aug 17, 2020

Yeah, virtual can be removed here.

@pramodk pramodk merged commit e9a7b19 into master Aug 18, 2020
@pramodk pramodk deleted the minor-fixes branch August 18, 2020 13:06
pramodk pushed a commit to neuronsimulator/nrn that referenced this pull request Nov 2, 2022
* Clean up interleave_info.
* No need to allocate nodeindices for artificial cells.
* Two allocation methods in use for int arrays in InterleaveInfo.
* No need for virtual destructor.

CoreNEURON Repo SHA: BlueBrain/CoreNeuron@e9a7b19
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants