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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 6 commits into from
Mar 4, 2022

Conversation

RalphSteinhagen
Copy link
Contributor

@RalphSteinhagen RalphSteinhagen commented Jan 28, 2022

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! 馃憤

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:

  • updated Catch2 to v2.13.8 (216713a) - the old version isn't compatible anymore with more modern OSes.
  • replacing C-style typedef type aliasing by 'using type = ' see also C++ Core Guidelines - T.43
    N.B. this should make later refactoring (and possible introduction of 'concepts') a bit easier ...
  • replaced std::is_same<...>::value by std::is_same_v<...>
  • replaced std::conditional<...>::type by std::conditional_t<...>

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.

attempt to trigger a build for this pr
@kirkshoop kirkshoop merged commit 7f97aa9 into ReactiveX:main Mar 4, 2022
@guhwanbae
Copy link
Contributor

@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
Copy link
Member

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

@guhwanbae
Copy link
Contributor

@kirkshoop , Yes,
79addf1
It replaced std::is_same::value with std::is_same_v alias since C++17.

@RalphSteinhagen
Copy link
Contributor Author

@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
Copy link
Member

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!

RalphSteinhagen added a commit to RalphSteinhagen/RxCpp that referenced this pull request Mar 14, 2022
RalphSteinhagen added a commit to RalphSteinhagen/RxCpp that referenced this pull request Mar 14, 2022
RalphSteinhagen added a commit to RalphSteinhagen/RxCpp that referenced this pull request Mar 14, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants