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

Use std::unique_ptr with containers #148

Closed
hugbug opened this issue Jan 10, 2016 · 2 comments

Comments

Projects
None yet
1 participant
@hugbug
Copy link
Member

commented Jan 10, 2016

For intro see #88.

For many containers the object ownership model was reworked in #143. For objects which cannot be stored directly in containers we still need to use pointers. To make the relations in code clear we should use the rules:

  • owned objects must be wrapped with std::unique_ptr to indicate ownership;
  • raw-pointers can be used only for non-owned objects.

@hugbug hugbug added the refactoring label Jan 10, 2016

@hugbug hugbug added this to the Modern C++ milestone Jan 10, 2016

@hugbug hugbug referenced this issue Jan 10, 2016

Closed

Modern C++ #88

21 of 21 tasks complete
@hugbug

This comment has been minimized.

Copy link
Member Author

commented Jan 13, 2016

Results

Refactored the whole code by wrapping all pointers stored in containers with std::unique_ptr and didn't like the result:

  • the logic which manages objects' lifetime has become more complex; that was surprising since std::unique_ptr was supposed to make that simpler;
  • sometimes objects (pointers) must be collected into temporary containers to make further manipulations; it has become not possible to use the same container classes for that; instead a separate classes (for raw pointers) were needed; there were few functions that needed to work with both container types and that required a separate and difficult rework of that parts (which I didn't completed);
  • having to deal with two pointer types for same objects made the things additionally complex (again);
  • the advantage of automatically destroying objects when removing them from containers has brought very little on the table; sometimes (pretty often actually) extra efforts were needed to move objects between containers.

All in one the code has become considerably more complex (and uglier a little bit). The goal of simplifying object lifetime management wasn't achieved. I decided to discard the changes.

Nonetheless I hope to use std::unique_ptr to hold individual objects (not within containers) where appropriate, such as local or member variables but that will be a separate issue tracker item.

@hugbug hugbug closed this Jan 13, 2016

@hugbug hugbug changed the title Indicate object ownership with std::unique_ptr Use std::unique_ptr with containers Jan 16, 2016

@hugbug hugbug reopened this Mar 10, 2016

@hugbug

This comment has been minimized.

Copy link
Member Author

commented Mar 11, 2016

Another attempt

After a few more attempts to rework containers for download objects (articles, files, nzbs, history) to hold unique_ptr instead of raw pointers I've finally made it.

Let's take a look at concerns stated in the previous post:

the logic which manages objects' lifetime has become more complex; that was surprising since std::unique_ptr was supposed to make that simpler;

After working with unique_ptrin #168 I'm feeling more confident using it.

sometimes objects (pointers) must be collected into temporary containers to make further manipulations; it has become not possible to use the same container classes for that; instead a separate classes (for raw pointers) were needed;

Very true. The solution was indeed to use other collection classes for temporary containers. I usually use vector<T*> but for few case I've even introduced special named classes like RawFileList.

there were few functions that needed to work with both container types and that required a separate and difficult rework of that parts (which I didn't completed);

The main such place was DiskState.cpp, where the reason for using both raw-containers and unique_ptr-containers was the conversion functions to read old file formats. I've "solved" this problem by discarding support of old disk state formats (#183).

having to deal with two pointer types for same objects made the things additionally complex (again);

The most noticeable places were for-range loops on containers. Since the loop variable receives unique_ptr it was necessary sometimes to get to the raw-pointer using .get()-method. That made code ugly. I've developed a solution using templates which automatically make for-range loops to receive raw-pointers (see Container.h).

All in one the code has become considerably more complex (and uglier a little bit). The goal of simplifying object lifetime management wasn't achieved.

The lifetime management has become a little bit easier due to removing of custom container destructors and Clear()-methods. More important is that the code gradually becomes more exception safer (although we don't use in exceptions in NZBGet yet).

hugbug added a commit that referenced this issue Mar 12, 2016

hugbug added a commit that referenced this issue Mar 12, 2016

@hugbug hugbug closed this Mar 12, 2016

hugbug added a commit that referenced this issue May 29, 2016

#148: fixed crash in queue script handling
If multiple queue scripts were queued and the nzb-file was deleted
before all scripts were executed the program may crash.

hugbug added a commit that referenced this issue Oct 9, 2017

#148: fixed crash in queue script handling
If multiple queue scripts were queued and the nzb-file was deleted
before all scripts were executed the program may crash.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.