0

After a few days of delaying this question and debugging I've found that If my code below is run after removing the vector from the union (and all of the code mentioning this vector) the seg. fault dissapears.

#include <string>
#include <vector>
#include <functional>

struct Task
{
  std::string Name;

  std::vector<Task*> *Subtasks;
  std::function<void(std::string const &)> Job;

  Task() {}
  Task(std::string const &arg_0) { Name = arg_0; }
  Task(std::string const &arg_0, std::vector<Task*> *arg_1) { Name = arg_0; Subtasks = arg_1; }
  Task(std::string const &arg_0, std::function<void(std::string const &)> arg_1)
  { Name = arg_0; Job = arg_1; }

  ~Task() { for (auto tItem : *Subtasks) { delete tItem; } }
};

class Console
{
private:
  std::vector<Task*> Routine;

public:
  ~Console() { for (auto tItem : Routine) { delete tItem; } } //I thought that this is not needed but Valgrind thinks otherwise, strangely the deconstructors of the Tasks are not called, I guess.

  void add(Task *arg_0) { Routine.push_back(arg_0); }

  void foo()
  {
    Task *tTask = new Task();
    //Task *tTask = new Task("Name");
    //Task *tTask = new Task("Name", [this](std::string const &arg_0){ ; });
   //Seg. fault still remains.
    add(tTask);
  }
};

int main()
{
    Console _Console;
    _Console.foo();
}

For quick online IDE code test

This should probably be another question, but I think it's too simple. I've heard before, that if a non-trivial is used in a union, it should be taken care of if the values are switched, how would one do that, and is it necessary if I do not intend to have any value changes on runtime?

EDIT1

Removed the union of vector and std::function

EDIT2

The Seg. Fault is most likely caused by the deconstructor ~Task();.

Community
  • 1
  • 1
niraami
  • 646
  • 8
  • 18
  • 3
    Between ignoring the Rule of 5/3/0 and the undefined behavior in handling your `union` I'd say you've got some problems son. – Captain Obvlious Sep 07 '16 at 19:51
  • What is the purpose of the union? Are you trying to save a little space? – NathanOliver Sep 07 '16 at 19:54
  • 1
    `Job = arg_1` puts junk in pointer in `Subtasks`, and crash when deleted. – Jean-François Fabre Sep 07 '16 at 19:55
  • @Jean-FrançoisFabre - But only if he ever invokes that constructor. Since he doesn't provide a `main()`, it's hard to know. – Robᵩ Sep 07 '16 at 19:57
  • @NathanOliver Hmm, you are right. I was planning to implement this header in a different way and I forgot that here, I don't need the union. – niraami Sep 07 '16 at 19:58
  • If you do not need it then I would just get rid of it. Solves at least half of your problems. – NathanOliver Sep 07 '16 at 19:58
  • @Robᵩ I provided it in the online IDE example, and yes, I did invoke it. But even after removing the union (that should resolve that problem), the seg. fault remains. – niraami Sep 07 '16 at 19:58
  • can you edit your post showing your fixes then? I bet you don't set `Subtasks=nullptr` in the `job` constructor: destructor still deletes undefined pointer. Classical mistake. – Jean-François Fabre Sep 07 '16 at 19:59
  • 2
    @areuz - Please provide a short, **complete** example in the question itself. See [mcve] for more information. – Robᵩ Sep 07 '16 at 20:00
  • 1
    Now that you have done that you need to include a [mcve] in the question so we can try and run it. Also if you know what line causes the segfault that would be nice to know. – NathanOliver Sep 07 '16 at 20:00
  • @NathanOliver Edited the question, I do not really know what causes it. But I will investigate more and update the question. – niraami Sep 07 '16 at 20:03
  • To avoid this problem, use vector of smart pointers, instead of pointer to vector of pointers. – M.M Sep 07 '16 at 20:49

1 Answers1

1

If this constructor is called:

Task(std::string const &arg_0, std::function<void(std::string const &)> arg_1)
{ Name = arg_0; Job = arg_1; }

then it crashes for sure when destructor is called because Subtasks is not initialized.

~Task() { for (auto tItem : *Subtasks) { delete tItem; } }

Fix:

Task(std::string const &arg_0, std::function<void(std::string const &)> arg_1)
{ Name = arg_0; Job = arg_1; Subtasks = nullptr;}

~Task() { if (Subtasks!=nullptr) for (auto tItem : *Subtasks) { delete tItem; } }

and (thx for the comments) gather/replace and fix all other constructors like this:

 Task(std::string const &arg_0 = "", std::vector<Task*> *arg_1 = nullptr) { Name = arg_0; Subtasks = arg_1; }

After some discussion, it appears that there's a possible design flaw in the code of the question: deleting the contents of the vector is dangerous since the pointers may be still in use by other clients, or may not even been allocated, but just reference local variables. And why Subtasks is a pointer in the first place? Why not deleting it?

Jean-François Fabre
  • 137,073
  • 23
  • 153
  • 219
  • In [his ideone.com example](http://ideone.com/i0p8jd), he calls the default constructor. It will need to initialize `Subtasks`, also. – Robᵩ Sep 07 '16 at 20:03
  • checking for `nullptr` is not needed, since deleting `nullptr` is perfectly safe – Arnav Borborah Sep 07 '16 at 20:08
  • 2
    Another thing that should be addressed is why `Task` deleting the contents of the vector but not the vector itself. Ownership here is not clear but `Task` is deleting the contents of the vector but not the vector itself. Using `std::unique_ptr` or `std::shared_ptr` here takes care of that, simplifies destroying the object (from a code writing perspective), and makes it easier to work with. – Captain Obvlious Sep 07 '16 at 20:19
  • @CaptainObvlious: you have a point. Either destroy everything or nothing. An empty destructor would probably be the good choice of design. – Jean-François Fabre Sep 07 '16 at 20:21
  • Amazing answer, and a lot of good points in the comments. I really do not know why the *Subtasks* was a pointer, as I said, I had a different approach in mind, but I changed it, as I actually wrote the code - That is probably the cause of all of my problems. Thanks for all of the answers. – niraami Sep 07 '16 at 20:31
  • A vector of pointers is useful though (polymorphic elements), but a pointer on a vector is generally asking for trouble :) – Jean-François Fabre Sep 07 '16 at 20:51