ReactiveX / RxCpp

Reactive Extensions for C++
Apache License 2.0
3.03k stars 390 forks source link

-Werror for unsafe nullptr - `decltype((*(input_type*)nullptr).out((*(Subscriber*)nullptr)))' evaluation #570

Closed RalphSteinhagen closed 2 years ago

RalphSteinhagen commented 2 years ago

First, thanks for this very interesting and well-thought-through library. :+1:

The shared.cmake prescribes strict type checking via -Werror for both gcc and clang. This fails for at least gcc 11.2.1, clang 13.0.0, and possibly earlier versions for a decltype(..) evaluation relying on an nullptr cast:

[..]/RxCpp/Rx/v2/examples/cep/main.cpp:34:12:   required from here
[..]/RxCpp/Rx/v2/src/rxcpp/rx-coordination.hpp:62:56: error: ‘this’ pointer is null [-Werror=nonnull]
   62 |         using type=decltype((*(input_type*)nullptr).out((*(Subscriber*)nullptr)));
      |                             ~~~~~~~~~~~~~~~~~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~

Maybe this pointer/type magic could be replaced by a safer alternative.

Since this library aims at newer compiler/C++ standards, maybe the various typedefs could possibly be replaced by using type=....

RalphSteinhagen commented 2 years ago

Am willing to get this working and to provide a PR (with guidance). I thus gave it a quick shot -- ignoring the warnings aside (for which the code works perfectly well -- I replace the offending pointer-casting-magic in the above and other similar locations by

using type = decltype(std::declval<input_type>().in(std::declval<Observable>()));

On that note, I am having two questions:

  1. what is the C++ standard that this library aims for? The README.md seems to indicate C++20 ([..] RxCpp depends on the latest compiler releases [..])
  2. why these type declarations? Wouldn't automatic type deduction (auto) or constraint types via concepts work equally? This would make the code easier to read/reason from a C++20 perspective and provide nicer compiler errors, wouldn't it?

Any feedback on this would be appreciated.

RalphSteinhagen commented 2 years ago

@kirkshoop or anybody else: any chance to get feedback on this and the corresponding PR?

We liked to use it in our C++20 project but the compiler warnings turned into errors would be a substantial technical burden.

Thanks in advance and much appreciated! :+1:

RalphSteinhagen commented 2 years ago

@kirkshoop thanks for merging and accepting this! Issue is fixed by PR closing now.