ETLCPP / etl

Embedded Template Library
https://www.etlcpp.com
MIT License
2.25k stars 392 forks source link

Allow etl::observer notification without argument #940

Closed jwellbelove closed 3 months ago

dhebbeker commented 3 months ago

I noticed that you added support for this specialization in b6801b5c0c615a18ba85dc0c9147ceb6fd65081f. Are there technical obstacles for adding support only to C++≥11? My preliminary tests (before your change) did not reveal an issue in respect with older C++ versions.

In the current version (20.39.0) this feature is not supported for C++<11 (test).

jwellbelove commented 3 months ago

In the Godbolt link for the preliminary test you have this.

namespace etl {
    template <>
  class observer<void>
  {
  public:
    virtual ~observer() = default;
    virtual void notification() = 0;
  };
}

The compiler option is set to -std=c++98, but the code uses = default; which is a C++>=11 feature. It looks like the code is using the C++>=11 code.

That said, I've tried the unit tests with ETL_FORCE_TEST_CPP03_IMPLEMENTATION defined and the C++<11 code (non-variadic implementation) compiles fine.

jwellbelove commented 3 months ago

But it does not pass with GCC (typical!)

jwellbelove commented 3 months ago

Even with adding the specialisation for void, the limitation with the current C++<11 code is that void can only be used on its own as a template parameter. To handle template type as with the C++>=11 version would require a rewrite of the C++<11 code. I'll add the specialisation, and look into the changes required for a rewrite.

jwellbelove commented 3 months ago

Actually, the re-write is relatively simple. The trick is to implement each multi-template parameter observer in terms of inheriting from multiple single template parameter definitions.

For example:-

  template <typename T1,
            typename T2,
            typename T3,
            typename T4>
  class observer<T1, T2, T3, T4> : public observer<T1>
                                 , public observer<T2>
                                 , public observer<T3>
                                 , public observer<T4>
  {
  public:

    virtual ~observer() {}
    using observer<T1>::notification;
    using observer<T2>::notification;
    using observer<T3>::notification;
    using observer<T4>::notification;
  };

  template <typename T1>
  class observer<T1>
  {
  public:

    virtual ~observer() {}
    virtual void notification(T1) = 0;
  };

  template <>
  class observer<void>
  {
  public:

    virtual ~observer() {}
    virtual void notification() = 0;
  };
dhebbeker commented 3 months ago

Great!

dhebbeker commented 3 months ago

The extensions you implemented in v20.39.1 for observer look good. But I think for the complete pattern to be functional with C++<11 observable must be extended.

Until now

void notify_observers()

Has only been added for C++≥11.

See test with Compiler Explorer.

jwellbelove commented 3 months ago

Fixed 20.39.3