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

Package version changing functionality of string_view #861

Open
debreczenikalman opened this issue Jan 6, 2025 · 6 comments
Open

Package version changing functionality of string_view #861

debreczenikalman opened this issue Jan 6, 2025 · 6 comments
Labels

Comments

@debreczenikalman
Copy link

Hi!

I have a little problem with this package.

The 3.0.1 -> 3.0.3 update changed the functionality how the DISABLE_STRING_VIEW (option in your CMakeLists.txt at line 38) is handled later on, at line 112. In version 3.0.1, if the setting is ON, it defines HAS_STRING_VIEW=0, otherwise it does not define said macro at all. But in v. 3.0.3, it is defined, no matter what, to either HAS_STRING_VIEW=0 or HAS_STRING_VIEW=1.

To have the issue summarised; the date.h header, at line 35 should automatically decide whether to define HAS_STRING_VIEW to 0 or 1. This automatic check is now unused/obsolete, but our project relied on this check.

My issue is the following: I'm working on a project with my team that uses different versions of C++. The Build 5 uses C++ 14, which has no <string_view> header, but Build 8 uses C++ 17, which has said header.

The problem arises by having that macro defined no matter what. I can't turn the setting to OFF, as it breaks build 5 (it tries to include the header, but there is none), having the setting ON breaks build 8 (it uses the string_view specific functions).

It would be rude from me to edit your date.h header without a pr first, and sadly, I don't have much control over our build system, as other projects use this as well, changing would break the rest of them. I'm right now trying to edit the CMakeListsCustom file, see if I can control the option this way. Fearing the worst, what other recommendations could you give us to fix the problem?

@HowardHinnant
Copy link
Owner

The logic for setting (or not setting) HAS_STRING_VIEW in date.h is set in stone. I recommend your current path of having your build system set HAS_STRING_VIEW externally. If you can do that, date.h will respect whatever setting you make.

Unfortunately I don't have the expertise to manage my own CMake files and it was a mistake for me to allow them to be added to this project in the first place. If you can adjust your build system to ignore my CMake files completely, that would be ideal.

@debreczenikalman
Copy link
Author

I fear there might be a misunderstanding here... The cmake defines HAS_STRING_VIEW every time. According to the code change in #766 , the macro will be defined no matter what the option is. The check in date.h might be set in stone, but it is in practice rendered obsolete, due to the macro always existing (macro is always defined, thus preprocessor will never do a check). But I'll go further; this also means every build that does not explicitly turns the option DISABLE_STRING_VIEW to ON, will be broken.

Because of the always defined state of HAS_STRING_VIEW, we would need two different builds of the date package. I introduced a temporal fix by changing the versions back to 3.0.1, but it can't last long sadly.

@HowardHinnant
Copy link
Owner

My CMake files are evil, sorry.

I would like to remove them completely. If I did that, would it make using my library easier or harder for you?

@debreczenikalman
Copy link
Author

As of right now, let's keep everything as is, because I don't really want to change an entire package's logic for thousands of people, just because we have an error. We are trying to find a workaround to override the result of this package's state. I'll keep you updated, and if we find something, hopefully someone else will find it useful too.

@debreczenikalman
Copy link
Author

Good news! We found a way to fix this, two actually. One is less than ideal... Either way, I suggest the community to re-open the #766 request and give it a second thought, as in my opinion, it shouldn't have been included, it was an honest mistake.

First, the dummy solution

This code block above every include knocks out the mistakenly defined variable

#ifdef HAS_STRING_VIEW
#undef HAS_STRING_VIEW
#endif
#include <date/date.h>

Second, the good solution

We had to introduce this line to our cmake file, in order to override the mistakenly defined macro

if (${_VERSION} LESS 7)
	set_target_properties(date::date PROPERTIES INTERFACE_COMPILE_DEFINITIONS "HAS_STRING_VIEW=0")
endif ()

@HowardHinnant
Copy link
Owner

Thank you for showing your solutions.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

2 participants