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

[date] Add official CMake targets support #8151

Merged
merged 1 commit into from
Sep 17, 2019
Merged

[date] Add official CMake targets support #8151

merged 1 commit into from
Sep 17, 2019

Conversation

qis
Copy link
Contributor

@qis qis commented Sep 12, 2019

Howard Hinnant added an official CMake script with target exports. Breaking changes:

  • The minimum C++ version is set to C++17.
  • Can't build database support on Linux without enabling remote API.
  • The unofficial:: prefix goes away, but a wrapper script is installed by this port.

I suggest removing the wrapper script after a transition period.

@qis
Copy link
Contributor Author

qis commented Sep 12, 2019

What's the best way to fix this?

https://github.com/HowardHinnant/date/blob/c56f915/include/date/date.h#L139

I'm surprised it's not disabled earlier in the header file.

https://github.com/HowardHinnant/date/blob/c56f915/include/date/date.h#L102

I could add a target_compile_definitions(date PUBLIC HAS_UNCAUGHT_EXCEPTIONS=1) or set it to PRIVATE and let the user deal with it.

Setting /Zc:__cplusplus would be a clean fix that we apply in a custom toolchain file, but that requires a relatively recent compiler and can't be exported since it is likely to break user code.

Edit: Talked to Howard. Will submit a PR to the date repo and update this PR if it gets accepted.

@cbezault cbezault added the depends:upstream-changes Waiting on a change to the upstream project label Sep 12, 2019
@qis
Copy link
Contributor Author

qis commented Sep 13, 2019

@dan-shaw We have fixed the C++ version detection in date.h.

The UWP build problem still persists since the library uses C++17 by default (which I find a good idea), but expects the compiler to still support std::unhandled_exception() since it's deprecated, not removed.

The library defines _SILENCE_CXX17_UNCAUGHT_EXCEPTION_DEPRECATION_WARNING on line 102 (checked to be sure), but I still get the following error:

date.h(1054,1): error C4996:  'std::uncaught_exception': warning STL4006: std::uncaught_exception() is deprecated in C++17. It is superseded by std::uncaught_exceptions(), plural. You can define _SILENCE_CXX17_UNCAUGHT_EXCEPTION_DEPRECATION_WARNING or _SILENCE_ALL_CXX17_DEPRECATION_WARNINGS to acknowledge that you have received this warning.

Strangely, this only happens when compiling for UWP targets.

What would be the appropriate fix for that? I can think of these:

  1. Patch date.h to use std::unhandled_exceptions() in C++17 mode.
  2. Patch date.h and add #pragma warning(disable : 4996) (personal favorite).
  3. Patch CMakeLists.txt to disable warning with /wd4996 (bad idea).
  4. Patch CMakeLists.txt to set the default C++ version to 14 (sounds reasonable).

Regarding option 4: I really like that you can overwrite this value in a custom toolchain file. The tz.h interface depends on it. Please don't force me to set it in portfile.cmake and break this functionality.

Is this a known MSVC bug or did I miss something?

Edit: I force-pushed option 2.

@cbezault cbezault removed the depends:upstream-changes Waiting on a change to the upstream project label Sep 16, 2019
Copy link
Contributor

@dan-shaw dan-shaw left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, the uwp patch looks fine - just one comment

@dan-shaw dan-shaw merged commit d4ac078 into microsoft:master Sep 17, 2019
@dan-shaw
Copy link
Contributor

Thanks for the PR!

@qis qis deleted the dev/qis/date branch September 18, 2019 06:53
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.

3 participants