search

HowardHinnant / date

A date and time library based on the C++11/14/17 <chrono> header
Other
3.09k stars 674 forks source link

HAS_UNCAUGHT_EXCEPTIONS not set for C++17 compilers #435

Open londey opened 5 years ago

londey commented 5 years ago

The test for C++17 was changed in the following commit from >= to >

https://github.com/HowardHinnant/date/commit/5ba1c1ad8514362dba596f228eb20eb13f63d948#diff-60920d5d6696dfe1f8a446d6e86869b2R130

std::uncaught_exception() is deprecated in C++17 and so fails to build cleanly on C++17 compilers such as GCC 8.2.

This version of the code is now live on vcpkg an causes builds using warnings as errors to fail.

Please change this test back to __cplusplus >= 201703 so that date.h builds cleanly on compliant compilers.

HowardHinnant commented 5 years ago

Ouch. This change was made to make people using macOS happy because they wanted to compile on the latest OS, but target earlier OS's that don't have std::uncaught_exceptions().

Best laid plans... ;-)

Would it be acceptable for you to include -DHAS_UNCAUGHT_EXCEPTIONS=1 on your compile command line?

londey commented 5 years ago

Thank you for your response.

I appreciate the difficulty of striking a balance between supporting older compilers and avoiding deprecation warnings on newer ones.

I had applied a similar workaround locally but still thought I would bring it to your attention that it was having issues out of the box.

rcdailey commented 4 years ago

I have opened a pull request to correct this bug. IMHO, the code should correctly use __cplusplus. If there are platforms and/or toolchains that incorrectly set that value, then those projects should be the ones that have to manually specify preprocessor directives at the command line. However, projects that use toolchains that correctly implement the language should not be required to do that.

Basically, your macOS people should be the ones explicitly defining HAS_UNCAUGHT_EXCEPTIONS=1.

HowardHinnant commented 4 years ago

uncaugh_exception() is required to be present in C++17.

rcdailey commented 4 years ago

That's true, but not my point. You aren't using std::uncaught_exceptions() in C++17 and you should. Sure, the singular version is available but deprecated. Some toolchains, such as MSVC, print a warning if you are using it with C++17. It's completely removed in C++20. These compiler warnings can cause code bases to fail to compile when warnings are treated as errors, like in my case.

The correct solution is to not rely on deprecated features.

HowardHinnant commented 4 years ago

Soon you will not have to use this library at all, and this won't be an issue. Your vendor will supply this library. :-)

rcdailey commented 4 years ago

You're implying that it will be available with C++20, right? The whole point of this, as I understand it, is to have access to first class date processing without modifying the language standard I use in my build system. If I am misunderstanding please let me know.

Otherwise, I see your response as an acknowledgement of the issue and refusal to allow it to be corrected (I have already provided the fix for you in a PR).

HowardHinnant commented 4 years ago

Yes, it is in the C++20 draft: http://eel.is/c++draft/#time

The whole point of this library was to get field experience with it so that it could be refined and then proposed for standardization with confidence. That goal has been achieved. The new goal is now to track the standard specification as closely as possible as it changes (more coming soon on that I hope).

On the HAS_UNCAUGHT_EXCEPTIONS issue, I have tried it both ways now. The field experience tells me that there is no way to make everyone happy. Changing this again will make you happy and then I'll hear from others who aren't.

So I have stopped trying to make everyone happy on this issue. Sorry that you are in the camp which isn't. But even so, I have made the issue configurable in order to maximize happiness. And I have trended the issue to move in the direction you would like as people migrate past C++17. And yet, as sure as I'm sitting here, I know deep down that I still can not make everyone happy.

Thank you for your active participation in making this a better library, and for your PR. But when I get back to processing PR's, I will, respectfully, be declining #470.

rcdailey commented 4 years ago

I appreciate your time. It seems I will have to wait for C++20. I thought this was a general purpose library, which was my mistake. Since the implementation of this library is incorrect, I will remove it from my code base.

Also I'm not really unhappy, I know there are ways to make it work, it just seems backwards to cater towards the broken use case. There are plenty of libraries out there that are correctly checking for C++17 with __cplusplus >= 201703 instead of __cplusplus > 201703. However I can agree to disagree on this.

I'll go ahead and close my PR to save you the trouble.