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

EAC-3238 remove use of deprecated C++17 features #17

Conversation

jstinson-te
Copy link
Contributor

@jstinson-te jstinson-te commented May 11, 2023

The std::result_of has been deprecated in C++17 and has been completely removed in C++20.

This PR detects the C++17 and uses the std::invoke_result when the detected C++ version is high enough.

This should resolve issues like the following seen on Windows,

09:58:32     12>Z:\.conan\9fb35c\1\include\thousandeyes/futures/then.h(80,32): warning C4996: 'std::result_of<te::unibrow::config::DefaultRestartableComponentManager::start::<lambda_2> (std::future<std::vector<std::future<void>,std::allocator<std::future<void>>>>)>': warning STL4014: std::result_of and std::result_of_t are deprecated in C++17.

Testing

Mac

Built tests and examples with C++17 and C++14, adding a compile time message to prove the correct code macro was compiled. Tests pass with no issues.

Windows

Built tests and examples with C++17 and C++14, adding a compile time message to prove the correct code macro was compiled. Tests pass with no issues.

As a library

Exported the library on Mac and built against the library with no issues.
Exported the library on Windows and built against the library with no issues. Also ran gtest tests as a sanity check, it passed without issues.

std::result_off is deprecated in c++ 17 and removed in c++20, it has
been replaced with std::invoke_result.
Changes include:

* Set C++ version once in head file (only used for tests and examples)
* Make a new tests and examples target to build all conveniently
* Add compile commands for ninja and cmake generators
* Bump C++ version for tests and examples to c++ 17
@jstinson-te jstinson-te marked this pull request as ready for review May 11, 2023 18:32
@jstinson-te jstinson-te requested review from a team and sudatar-te and removed request for a team May 11, 2023 18:32
@jerriley-te
Copy link

@jstinson-te Do we need to continue to support C++14?

@jstinson-te
Copy link
Contributor Author

@jstinson-te Do we need to continue to support C++14?

I believe this library is potentially in use by other projects so I was being cautious. @ggeorgalis are you able to comment on this. Happy to throw away the C++14 support if it is not needed.

@jstinson-te
Copy link
Contributor Author

@jstinson-te Do we need to continue to support C++14?

I believe this library is potentially in use by other projects so I was being cautious. @ggeorgalis are you able to comment on this. Happy to throw away the C++14 support if it is not needed.

Discussed with @ggeorgalis 1-to-1, we decided that c++14 support is required as there are other teams that use this.

Also, this change was proposed before, see #16 this approach is essentially the same, however I believe my macro is more robust to determine the C++ version.

@jstinson-te
Copy link
Contributor Author

jstinson-te commented May 24, 2023

CI is broken due to an archived repo which has a bad label reference, see https://github.com/thousandeyes/conan-thousandeyes-futures

@jstinson-te
Copy link
Contributor Author

Discussed briefly with @ggeorgalis & @jerriley-te and we agreed to merged.

@jstinson-te jstinson-te merged commit dc16236 into thousandeyes:master May 24, 2023
@jstinson-te jstinson-te deleted the EAC-3238-Remove_Use_Of_Deprecated_C++17_features branch January 29, 2024 17:34
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

2 participants