1
\$\begingroup\$

Can this be made more C++ a like with std::list or something so the Clink class is not needed?

I have a dynamic list of objects, and the number of them could change during runtime.

The expected output in no specific order. As an example 'Cb' runs before 'Ca':

link-contructed
Ca-contructed
Cb-contructed
Cb-run
Ca-run
Ca-destructed
Cb-destructed
link-destructed

I would like the program to be more C++ like with std::list or something. I have looked at std::forward_list but can not really see how it should be used. All examples I could find is about std::list<int>. My Ctask needs to have next in it and Clink needs to know which objects is used. Not so generic. Here is the mixed C/C++ program which does the expected:

/*
vim: ts=4 :
g++ -lstdc++ -o linked linked.cpp && ./linked
*/

#include <iostream>
using namespace std;

class Ctask {
public:
    Ctask *next = NULL;
    Ctask(void) {}
    virtual ~Ctask(void) {}
    virtual void run(void) = 0;
};

/* --------------------------------- */

class Clink {
public:
    Clink(void) {
        cout << "link-contructed" << endl;
    }
    ~Clink(void) {
        Ctask *temp;
        while (temp = head) {
            head = head->next;
            delete temp;
        }
        cout << "link-destructed" << endl;
    }
    void run(void) {
        Ctask *temp = head;
        while (temp) {
            temp->run();
            temp = temp->next;
        }
    }
    void add(Ctask *newnode) {
        if (tail)
            tail->next = newnode;
        else
            head = newnode;
        tail = newnode;
    }
private:
    Ctask *head = NULL;
    Ctask *tail = NULL;
};

/* --------------------------------- */

class Ca: public Ctask {
public:
    Ca(void) {
        cout << "Ca-contructed" << endl;
    }
    ~Ca(void) {
        cout << "Ca-destructed" << endl;
    }
    void run(void) {
        cout << "Ca-run" << endl;
    }
};

class Cb: public Ctask {
public:
    Cb(void) {
        cout << "Cb-contructed" << endl;
    }
    ~Cb(void) {
        cout << "Cb-destructed" << endl;
    }
    void run(void) {
        cout << "Cb-run" << endl;
    }
};

/* --------------------------------- */

int main(void) {
    Clink link;
    link.add((Ctask *)new Ca());
    link.add((Ctask *)new Cb());
    link.run();
    return 0;
}
\$\endgroup\$
  • 1
    \$\begingroup\$ Is it required that the destruction of elements happen in a specified order? If yes, I might have bad news for you. \$\endgroup\$ – Ben Steffan Feb 17 '18 at 19:44
  • \$\begingroup\$ @BenSteffan, all implementations I know of do linear destruction from the start. It is not guaranteed by the standard. Am I looking in the wrong way? (Not author of the question) \$\endgroup\$ – Incomputable Feb 17 '18 at 19:53
  • \$\begingroup\$ @BenSteffan Specified order would be best but unordered would be very useful. \$\endgroup\$ – hschou Feb 17 '18 at 19:54
  • \$\begingroup\$ @Incomputable Well that's the issue. In theory, an implementation might have a different order (although the most common implementations don't). \$\endgroup\$ – Ben Steffan Feb 17 '18 at 20:07
  • 1
    \$\begingroup\$ Note that td::forward_list<std::function<void()>> (of any other container storing captured closures) would probably do as well for this particular design, saving you some work of defining basic abstract class—with a list implementation detail inside!—and its children. \$\endgroup\$ – bipll Feb 20 '18 at 13:53
3
\$\begingroup\$

Based on your answer and the comments from @user673679, with some modifications:

  • Removing using namespace std.
  • Using unique_ptr to transparently handle object allocation and deallocation. Remember to create items with make_unique.
  • forward_list is just fine for a FIFO queue.

Note that I don't use a range-based for loop because I only iterate the list once (run function and remove element).

#include "ctask.h" // Ctask, Ca and Cb definitions
#include <forward_list>
#include <iterator>
#include <memory>

class TaskQueue {
public:
   TaskQueue() :
     _tasks(),
     _last( _tasks.before_begin() )
   {
   }

   ~TaskQueue() = default;

   void add( std::unique_ptr<Ctask>&& t ) {
     _last = _tasks.insert_after( _last, std::move(t) );
   }

   void run_all() {
     task_list::iterator prev = _tasks.before_begin();
     task_list::iterator curr = _tasks.begin();
     while( curr != _tasks.end() ) {
        (*curr)->run();
        curr = _tasks.erase_after(prev);
     }
     _last = prev;
   }

private:
   typedef std::forward_list<std::unique_ptr<Ctask>> task_list;
   task_list           _tasks;
   task_list::iterator _last;
};

int main(void) {
    TaskQueue queue;
    queue.add(std::make_unique<Ca>());
    queue.add(std::make_unique<Cb>());
    queue.run_all();
    return 0;
}

Edit: I realized that you can make run_all more readable since you are basically removing all the elements from the queue, so that the while loop doesn't actually depend on _last value. The simplified function would be:

   void TaskQueue::run_all() {
     while( !_tasks.empty() ) {
        _tasks.front()->run();
        _tasks.pop_front();
     }
     _last = _tasks.before_begin();
   }
\$\endgroup\$
  • \$\begingroup\$ I like the encapsulation of TaskQueue and run_all(). It was actually my next question: How to make the main more readable. Thanks. \$\endgroup\$ – hschou Feb 20 '18 at 13:53
  • \$\begingroup\$ You could make TaskQueue more efficient by making the list intrusive, so that you only need to dereference 1 pointer (next) instead of two (next and task data). In this case, Ctask would contain a member list_hook that will contain the pointer to the next element. \$\endgroup\$ – Jorge Bellon Feb 21 '18 at 9:37
1
\$\begingroup\$

After looking at a lot of std::list examples I came up with this solution:

#include <iostream>
#include <list>
#include <iterator>
#include <memory>
using namespace std;

class Ctask {
public:
    Ctask(void) {}
    virtual ~Ctask(void) {}
    virtual void run(void) = 0;
};

/* --------------------------------- */

class Ca: public Ctask {
public:
    Ca(void) {
        cout << "Ca-contructed" << endl;
    }
    ~Ca(void) {
        cout << "Ca-destructed" << endl;
    }
    void run(void) {
        cout << "Ca-run" << endl;
    }
};

class Cb: public Ctask {
public:
    Cb(void) {
        cout << "Cb-contructed" << endl;
    }
    ~Cb(void) {
        cout << "Cb-destructed" << endl;
    }
    void run(void) {
        cout << "Cb-run" << endl;
    }
};

/* --------------------------------- */

int main(void) {
    std::list<std::unique_ptr<Ctask>> taskList;
    taskList.push_back(std::make_unique<Ca>());
    taskList.push_back(std::make_unique<Cb>());
    for (auto& t : taskList)
        t->run();
    return 0;
}
  1. What would be a more precise question to the problem?
  2. Is this the correct & un-ugly way to do it?
  3. Can (*tl)->run() be written nicer?
  4. Can delete (*tl) be handled differently and should one avoid to directly call delete?
\$\endgroup\$
  • 2
    \$\begingroup\$ 2. Pretty much. 3. A range-based for loop: for (auto& t : taskList) t->run(); 4. Change the list to a std::list<std::unique_ptr<Ctask>> so the list owns the tasks, and there would be no need to do manual deletion. \$\endgroup\$ – user673679 Feb 20 '18 at 11:00
  • \$\begingroup\$ The technically correct implementation would be with a forward_list, where you remove items from the head and save an iterator to the tail, which is updated after every insertion. This would be a good step if you plan to insert and remove items from the list concurrently (parallel creation and execution). \$\endgroup\$ – Jorge Bellon Feb 20 '18 at 11:42
  • \$\begingroup\$ @user673679 Wow, that main code became rather short. I guess it can not be shorter. \$\endgroup\$ – hschou Feb 20 '18 at 13:36

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.