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

Allow etl::observer notification without argument #940

Closed
jwellbelove opened this issue Jul 31, 2024 · 8 comments
Closed

Allow etl::observer notification without argument #940

jwellbelove opened this issue Jul 31, 2024 · 8 comments
Assignees

Comments

@jwellbelove
Copy link
Contributor

No description provided.

@dhebbeker
Copy link
Contributor

I noticed that you added support for this specialization in b6801b5. 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
Copy link
Contributor Author

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
Copy link
Contributor Author

But it does not pass with GCC (typical!)

@jwellbelove
Copy link
Contributor Author

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
Copy link
Contributor Author

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
Copy link
Contributor

dhebbeker commented Aug 2, 2024 via email

@dhebbeker
Copy link
Contributor

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
Copy link
Contributor Author

Fixed 20.39.3

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Development

No branches or pull requests

2 participants