search

ReactiveX / RxCpp

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

replace C-style pointer cast type deduction by 'std::declval<>()' #572

Closed RalphSteinhagen closed 2 years ago

RalphSteinhagen commented 2 years ago

First of all: thanks for RxCpp and the great work you put into it! It's a really neat and useful library and I wonder why it isn't already part of the C++ standard! :+1:

This PR primarily addresses issue #570 and replaces pointer-type casting which causes warnings and (w/ -Werror) errors on newer compilers.

While refactoring, I noticed that some structures appear rather old (pre C++11) and could be simplified which I hope may help readability, modernizing towards C++20 (if this isn't excluded), and help new developers. Some of the low-hanging fruits (w/o breaking the apparent <=C++14 constraint) were:

I didn't tackle the ubiquitous std::enable_if<...>::type by std::enable_if_t<...> simplification and swapping of confusing ternary statements because of the sheer number of lines that would be impacted and because many of these lines wouldn't probably be needed anymore if this library would adopt C++20 and concepts (see also C++ Core Guidelines - T.48).

Let me know what you think and hope this fits the coding standard and style this library aims for.

guhwanbae commented 2 years ago

@RalphSteinhagen , @kirkshoop A while ago, I found that C++ standard version is not specified in the CMakeLists.txt file for unit tests. So there unit test are not compiled with gcc-9. gcc-9 use C++14 as default standard version.

So, Is C++17 minimum standard version of this project now?

kirkshoop commented 2 years ago

@guhwanbae - no, I do not intend rxcpp to drop cpp14 support yet. Did I merge something that requires c++17?

guhwanbae commented 2 years ago

@kirkshoop , Yes, https://github.com/ReactiveX/RxCpp/pull/572/commits/79addf123c6b94b8bda85099ae96b4f69919247b It replaced std::is_same::value with std::is_same_v alias since C++17.

RalphSteinhagen commented 2 years ago

@guhwanbae @kirkshoop I am terribly sorry. It was not my intention to implicitly boost to C++17. I relied on the existing cmake definitions and CI/CD definitions which neither reported errors nor warnings and it thus slipped through, sorry.

Since this is a very simple alias definition and in order to move forward, one could a) provide an alias (checking for the GCC/clang capabilities) and define an std::is_same_v<...> if <C++17, or b) search-and-replace each std::is_same_v<...> occurrence back to `std::is_same<...>::value.

The first is a bit dirty if RxCpp is targeted to remain at C++14 compatibility for the foreseeable future, and the second -- well a bit verbose.

@kirkshoop In any case, I'd be happy to make a PR to address this issue either way... just let me know your preference.

kirkshoop commented 2 years ago

No worries. I was also trusting the ci to cetch these types of issues.

I would prefer a local definition of rxcpp::is_same_v with a check for the std version macro for std::is_same_v. If the library has it alias it, if it does not, define it.

I would also like to have the ci enforcement strengthened so that it catches these things.

Thanks!