Iterator interface

[vaczijani]

Added the iterator interface to RegistrarBase class and changed the vector to static to avoid needed to handle nullptr.
This is my very first contribution to any github project, so let me know if I'm doing this incorrectly.
Best regards,
Janos

[wolframroesler]

Looks very good, thanks! Will review and test it, and maybe rework it to match naming conventions etc.

[vaczijani]

Shame on me! Recently read your Coding Standards and agreed with it. Will check back to make better work next time. Didn't want to create extra work for you. Many thanks.

[wolframroesler]

I find that empty pair of parentheses un-intuitive. I understand why it's needed, but one would expect the iterator to work as usual, i. e. x->dosomething();, as in the original example.

[vaczijani]

I agree. I don't like that form at all either. I noticed that down-casting is needed to get the right type object back.

[wolframroesler]

This calls the std::vector ctor which may throw, which would make the RegistrarBase ctor violate its noexcept promise. I think, however, that the static object hidden in a private function is superior to the smart pointer used previously.

[vaczijani]

My bad. I only considered the allocation in the original code and not push_back which obviously can trigger reallocation which can throw bad_alloc. try-catch is needed then.

[wolframroesler]

Is this use of auto supported in C++11 already?

[vaczijani]

My bad. Haven't considered C++11. Enjoyed C++17 given in the cmake file. I should have remembered the comment was saying it didn't really needed C++17.

[wolframroesler]

My bad as well because I forgot to change from 17 to 11 in the cmake file. This was originally written for a C++17 application, and I never bothered to test it on older versions until the requirement to use it in 11 came up recently.

[wolframroesler]

Compared to the for loop above, this iterator interface is much more complex to use, and the type name needs to be repeated. It would be nice to have something as elegant as the range-for syntax that still didn't have to create the std::vector returned by the plugins() function.

[vaczijani]

Agreed. Typing it was very painful, I wouldn't want anyone else needing to do it.

[wolframroesler]

push_back may throw, violating the noexcept promise.

[vaczijani]

I can see the difficulties with the current class hierarchy.
Would it be acceptable to do the downcasting in the RegistrarBase class and leave the Registrar class out?
Like:

#include <iostream>
#include <vector>
#include <algorithm>

namespace linktimeplugin {
// Register class keeps a collection of Plugins
// which you can retrieve either
// - Register<Plugin>::getPlugins()
// or
// - Register<Plugin>::begin(), Register<Plugin>::end()
// Plugin must be a subclass of Register!
// e.g. class PluginInterface : public Register<PluginInterface> {...}
template<class Plugin>
class RegistrarBase {
public:
    RegistrarBase() { registerPlugin(this); }
    // rule of 5
    RegistrarBase(const RegistrarBase&) = delete;
    RegistrarBase(RegistrarBase&&) = delete;
    RegistrarBase& operator=(const RegistrarBase&) = delete;
    RegistrarBase& operator=(RegistrarBase&&) = delete;
    virtual ~RegistrarBase() = default;

    typedef std::vector<Plugin*> Collection;
    typedef typename Collection::const_iterator const_iterator;
    // provide only const data for user
    static const_iterator begin() { return plugins().begin(); }
    static const_iterator end() { return plugins().end(); }

    static const Collection& getPlugins() { return plugins(); }

private:
    void registerPlugin(RegistrarBase<Plugin>* reg) {
        plugins().push_back(static_cast<Plugin*>(reg));
    }

    static Collection& plugins() {
        static Collection singleton;
        return singleton;
    }
};

// convenience macros:
// REGISTER_PLUGIN clearly indicates intent
#define REGISTER_PLUGIN(x) static x x##Instance
// DEFINE_PLUGIN_INTERFACE avoids repetition of plugin class name
#define DEFINE_PLUGIN_INTERFACE(x) class x : public RegistrarBase<x>


// Actual plugin interface defines the methods can be invoked
// on the given plugin. Has to inherit from Register class
// and tell its own type in template parameter.
DEFINE_PLUGIN_INTERFACE(PrinterPlugin) {
public:    
    virtual void print() = 0;
};

// Concrete plugins
class Cat : public PrinterPlugin {
public:
    virtual void print() override {
        std::cout << "meow\n";
    }
};

REGISTER_PLUGIN(Cat);

class Dog : public PrinterPlugin {
public:
    virtual void print() override {
        std::cout << "wuf\n";
    }
};

REGISTER_PLUGIN(Dog);

// A different plugin interface
DEFINE_PLUGIN_INTERFACE(OtherPlugin) {
public:
    virtual void move() = 0;
};

// Concrete plugin
class Snake : public OtherPlugin {
public:
    virtual void move() override {
        std::cout << "slither\n";
    }
};

REGISTER_PLUGIN(Snake);

} // namespace

int main() {
    typedef linktimeplugin::RegistrarBase<linktimeplugin::PrinterPlugin> Printers;
    std::cout << "using collection and range:\n";
    for(const auto p : Printers::getPlugins()) {
        p->print();
    }

    std::cout << "using iterator:\n";
    std::for_each(Printers::begin(), Printers::end(), 
        [](linktimeplugin::PrinterPlugin* p) {
            p->print();
        }
    );

    std::cout << "another type of plugin:\n";
    typedef linktimeplugin::RegistrarBase<linktimeplugin::OtherPlugin> Others;
    std::for_each(Others::begin(), Others::end(),
        [](linktimeplugin::OtherPlugin* p) {
            p->move();
        }
    );
}

Notice that I've let the constructor to throw exception: this can go into try/catch if unacceptable. I would rather let the program to throw exception on that because if extending the vector of pointers fails then we may have severe problem. Obviously I appreciate that you chose not to throw expcetion from the constructor for a very good reason.

[vaczijani]

Reorganised RegistrarBase and removed Registrar class to make the usage more convenient. Ready for an other review.

[vaczijani]

I'm not familiar with git. Sorry I don't understand what that confict is. I thought I've done everything I needed to resolve it.

git checkout master
git pull
git checkout iterator-interface
git merge master
git push origin iterator-interface
<Everything up-to-date

[wolframroesler]

Hi Janos, sorry for the late reply, haven't yet gotten around to checking your contribution. Never mind the conflict, I'll sort things out when the change is finished.

…

On Fri, Jun 18 2021 at 03:42:47 -0700, Janos Vaczi ***@***.***> wrote: I'm not familiar with git. Sorry I don't understand what that confict is. I thought I've done everything I needed to resolve it. > git checkout master > git pull > git checkout iterator-interface > git merge master > git push origin iterator-interface > <Everything up-to-date > — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#1 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AGDBQLJBO5APJ6LLWYVBXZLTTMPKPANCNFSM45PGPW5Q>.