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

Compilation Error on Clang 5.0 Upgrade #735

Closed
ErikPartridge opened this issue Sep 10, 2017 · 25 comments
Closed

Compilation Error on Clang 5.0 Upgrade #735

ErikPartridge opened this issue Sep 10, 2017 · 25 comments
Labels
solution: wontfix the issue will not be fixed (either it is impossible or deemed out of scope)

Comments

@ErikPartridge
Copy link

Upgrading to Clang++-5.0 on Travis-CI Trusty is producing the following error:


json/src/json.hpp:9869:66: error:  no type named 'string_view' in namespace 'std'
  ...and not std::is_same<ValueType, typename std::string_view>::value

Also tested on Ubuntu 16.04 Clang++-5.0 standalone without the rest of the project and still failing with the same error. g++ working fine on both systems.

@nlohmann
Copy link
Owner

Which library version are you using?

@ErikPartridge
Copy link
Author

This is only happening on the develop branch-- release 2.1.1 works fine.
using libstdc++ 5.4.1

@nlohmann
Copy link
Owner

Strange. I'm using clang version 5.0.0-svn312333-1~exp1 (branches/release_50) on Travis (see https://travis-ci.org/nlohmann/json/jobs/273804833 and https://travis-ci.org/nlohmann/json/jobs/273804832) without problems.

@nlohmann
Copy link
Owner

Seems related to #586 and #464. Do you use the latest develop version?

@ErikPartridge
Copy link
Author

Yup, I'm at fcba9ec.

I'm using the same version of clang with the -std=c++17 flag.

@nlohmann
Copy link
Owner

Could you please try to include <string_view>?

@ErikPartridge
Copy link
Author

<string_view> doesn't work, included <experimental/string_view> and it compiles perfectly!

@nlohmann
Copy link
Owner

Alright. Then there is something incomplete in a SFINAE. I don't want to include string_view - I just want to make sure I don't try to convert to/from it. I see if I find a way and may come back to you to test it since I cannot reproduce the error myself.

@nlohmann nlohmann added the solution: proposed fix a fix for the issue has been proposed and waits for confirmation label Sep 13, 2017
@nlohmann
Copy link
Owner

nlohmann commented Oct 2, 2017

I still cannot reproduce the issue. It seems that the SFINAE in

#if (defined(__cplusplus) && __cplusplus >= 201703L) || (defined(_MSC_VER) && _MSC_VER >1900 && defined(_HAS_CXX17) && _HAS_CXX17 == 1) // fix for issue #464
                   and not std::is_same<ValueType, typename std::string_view>::value
#endif

needs to "know" std::string_view, and this fails without including experimental/string_view. When I run clang 5.0, __cplusplus is defined to 201703L and the above code works without including any header for string_view.

@nlohmann nlohmann added state: help needed the issue needs help to proceed and removed solution: proposed fix a fix for the issue has been proposed and waits for confirmation labels Oct 2, 2017
@gregmarr
Copy link
Contributor

gregmarr commented Oct 2, 2017

You are probably seeing differences in the STL about which headers are included by other headers, or which classes are forward declared. The official position of the committee is that you MUST include all headers for classes that you use. To do otherwise is to risk compile breaks when you change compilers, or even versions.

@nlohmann
Copy link
Owner

nlohmann commented Oct 2, 2017

This would mean we need to include the required headers for std::string_view to exclude it via SFINAE, right?

@gregmarr
Copy link
Contributor

gregmarr commented Oct 2, 2017

you might be able to use SFINAE to check for the existence of it first, but then you might run into ODR issues if some TUs know about it before including json.hpp and others don't.
.

@nlohmann
Copy link
Owner

nlohmann commented Oct 5, 2017

In GCC 7.2.0, <string> includes bits/basic_string.h which contains

#if __cplusplus > 201402L
# include <string_view>
#endif

In Clang 5.0.0, <string> contains

#include <string_view>

I do not understand why @ErikPartridge 's Clang does not know about std::string_view (as json.hpp includes <string>). What am I missing?

@xirius
Copy link

xirius commented Oct 11, 2017

For me with clang++:


$ clang++ --version
clang version 6.0.0-svn315197-1~exp1 (trunk)
Target: x86_64-pc-linux-gnu
Thread model: posix
InstalledDir: /usr/bin


The header is in #include <experimental/string_view>
and string_view is under std::experimental::string_view

@nlohmann
Copy link
Owner

@xirius Thanks! Can you compile the library's tests with your clang version or do you experience the same error?

@xirius
Copy link

xirius commented Oct 11, 2017

It compiles.


mkdir build
cd build
cmake -DCMAKE_CXX_COMPILER=/usr/bin/clang++ ..

-- The CXX compiler identification is Clang 6.0.0
-- Check for working CXX compiler: /usr/bin/clang++
-- Check for working CXX compiler: /usr/bin/clang++ -- works
-- Detecting CXX compiler ABI info
-- Detecting CXX compiler ABI info - done
-- Detecting CXX compile features
-- Detecting CXX compile features - done
-- Configuring done
-- Generating done
-- Build files have been written to: /mnt/dev/json/build

cmake --build .
...
ctest
...
100% tests passed, 0 tests failed out of 70

Label Time Summary:
all = 186.48 sec (35 tests)
default = 8.70 sec (35 tests)

Total Test time (real) = 195.22 sec

@nlohmann
Copy link
Owner

@ErikPartridge What confuses me is that your Clang 5 relies on <experimental/string_view>. Shouldn't <string_view> be supported?

@xirius
Copy link

xirius commented Oct 12, 2017

Well, clang is a compiler and string_view is part of the standard library. So installing clang 5 (or 6 dev branch) doesn't bring the c++17 headers/lib(std)c++ (well at least not on Ubuntu/Debian and the like).

@nlohmann
Copy link
Owner

Well, it is hard to support a compiler without C++17 headers, but with the -std=c++17 flag.

@theodelrieu
Copy link
Contributor

Could we use the __has_include feature to detect that?

@nlohmann
Copy link
Owner

That may work. Then I would need to check for <string_view> and <experimental/string_view>.

@nlohmann
Copy link
Owner

Related: #795

@nlohmann
Copy link
Owner

I am still not sure whether this should be fixed. I mean: why should this library include <experimental/string_view> just to rule out errors in this combination of a C++17 compiler without a C++17 standard library?

@nlohmann nlohmann added state: please discuss please discuss the issue or vote for your favorite option and removed state: help needed the issue needs help to proceed labels Oct 27, 2017
@jjl
Copy link

jjl commented Oct 28, 2017

In principle, I agree. In practice, I'm one of the affected because the clang distribution and repos don't include libc++ and I haven't gotten around to building my own. It would be nice if I didn't have to.

@theodelrieu
Copy link
Contributor

One workaround (though quite brittle), would be:

// FixedJson.hpp (your header)
#include <experimental/string_view>
#include <json.hpp>

Then you include this file instead of including json.hpp directly. Of course once your distribution ships a library with C++17 support, you will not need that workaround anymore.

@nlohmann nlohmann added solution: wontfix the issue will not be fixed (either it is impossible or deemed out of scope) and removed state: please discuss please discuss the issue or vote for your favorite option labels Oct 28, 2017
rbns pushed a commit to rbns/ews-cpp that referenced this issue Sep 10, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
solution: wontfix the issue will not be fixed (either it is impossible or deemed out of scope)
Projects
None yet
Development

No branches or pull requests

6 participants