Re: [boost] [review] outcome broken on clang/libstdc++ (Linux) and Apple clang/libc++ and gcc 7.2
I wrote a .travis.yml for your boost-outcome submission to run your unit tests against a few compilers: The output is at: https://travis-ci.org/glenfe/ned14.outcome/builds/334222400 I updated it to make sure the clang++-libstdc++ uses a more recent libstdc++. It looks like: - clang++-libc++ fails because of forgetting to #include a header. - GCC 7.2 with -std=c++17 is the one with the Internal Compiler Errors (I didn't investigate further) - clang++5-libstdc++ fails because it cannot find a type in std Is there a reason why you don't have travis and appveyor for boost-outcome? I remember your "Best Practices" document strongly advocating that every Boost library submission should have them. Glen
On 27/01/2018 23:22, Glen Fernandes via Boost wrote:
I wrote a .travis.yml for your boost-outcome submission to run your unit tests against a few compilers:
The output is at: https://travis-ci.org/glenfe/ned14.outcome/builds/334222400
I updated it to make sure the clang++-libstdc++ uses a more recent libstdc++.
It looks like: - clang++-libc++ fails because of forgetting to #include a header.
Yep, that's a bug. Fixed in develop branch. Thanks for reporting it!
- GCC 7.2 with -std=c++17 is the one with the Internal Compiler Errors (I didn't investigate further)
GCC constexpr ICE bug described in previous email.
- clang++5-libstdc++ fails because it cannot find a type in std
On C++ 17, it uses std::in_place_type<T> instead of defining a local copy. This error suggests that either the STL being used doesn't define an implementation, or as you say a header is missing. Yet at https://github.com/ned14/boost-outcome/blob/master/include/boost/outcome/det..., it very clearly is including the correct header, and at https://github.com/ned14/boost-outcome/blob/master/include/boost/outcome/det... it is correctly selecting std::in_place_type on the value of __cplusplus. So on the basis of that, I'm going to assert that the libstdc++ version you are using is insufficiently new.
Is there a reason why you don't have travis and appveyor for boost-outcome? I remember your "Best Practices" document strongly advocating that every Boost library submission should have them.
https://travis-ci.org/ned14/boost-outcome https://travis-ci.org/ned14/outcome https://ci.appveyor.com/project/ned14/outcome Niall -- ned Productions Limited Consulting http://www.nedproductions.biz/ http://ie.linkedin.com/in/nialldouglas/
On Sat, Jan 27, 2018 at 6:43 PM, Niall Douglas wrote:
On 27/01/2018 23:22, Glen Fernandes via Boost wrote:
I wrote a .travis.yml for your boost-outcome submission to run your unit tests against a few compilers:
The output is at: https://travis-ci.org/glenfe/ned14.outcome/builds/334222400
Is there a reason why you don't have travis and appveyor for boost-outcome? I remember your "Best Practices" document strongly advocating that every Boost library submission should have them.
https://travis-ci.org/ned14/boost-outcome
The first one is for boost-outcome, correct? It only tests g++-6 with c++14, not g++-7 (in 14 or 17) (where the ICE occurs), or or clang+libc++ (where your other errors were). Aren't the second and third for standalone outcome? That isn't being reviewed here, correct? (i.e. Standalone outcome doens't even ICE MSVC2017 with Vinnie's test case, but boost-outcome, the one being submitted for review, does?). Glen
On 27 January 2018 at 17:48, Glen Fernandes via Boost wrote: ... where the ICE occurs... Any ICE is a compiler issue... so this is the wrong place to report the
(ICE-)issue...
gcc-7.3 has just been released
https://gcc.gnu.org/ml/gcc/2018-01/msg00197.html fixing over 99 bugs...
degski
On Sat, Jan 27, 2018 at 7:01 PM, degski via Boost wrote:
Any ICE is a compiler issue... so this is the wrong place to report the (ICE-)issue...
Sure. The author putting a library forward for review should feel free to report any compiler defects to vendors that his library exposes. It looks like Niall has done that already. Or are you suggesting that reviewers trying to review the submission during the review (by, among other things, running tests) shouldn't mention to the author that they encountered an ICE? Glen
On 27 January 2018 at 18:07, Glen Fernandes via Boost wrote: ... shouldn't mention to the author that they encountered an ICE? Sure you can mention it, but stating the obvious, the compiler shouldn't
crash, but should instead tell you what's wrong with the code (if anything
*is* actually wrong with the code, as it ICE-es we don't know, the only
thing we do know is that gcc-7.2 and lower won't handle it at all)...
degski
On Sat, Jan 27, 2018 at 7:15 PM, degski wrote:
Sure you can mention it, but stating the obvious, the compiler shouldn't crash, but should instead tell you what's wrong with the code (if anything *is* actually wrong with the code, as it ICE-es we don't know, the only thing we do know is that gcc-7.2 and lower won't handle it at all)...
Sure. It would be nice if compilers didn't have defects at all. Be they defects which result in an ICE, or defects that result in incorrect or sub-optimal code being generated, failure to conform to the specification, defective implementation of certain language or library features. One of the things I appreciated about Boost when I came to know Boost libraries more intimately when we started using them at Microsoft, was that they compensated for those defects (to the point where we would use Boost equivalents even instead of our C++ implementation's standard library facilities). Glen
the only thing we do know is that gcc-7.2 and lower won't handle it at all)...
My main development environment is MSVC on Windows as it's the lowest common denominator usually. Then it's GCC on Linux. Finally, only occasionally, I give it a whirl on clang on FreeBSD. Outcome may cause GCC to sometimes ICE in its unit tests, but really I assure you that GCC is a fine compiler to write Outcome based code with. You very rarely encounter problems in normal coding, and GCC has a Concepts implementation which Outcome uses and makes for much more useful compiler error messages, plus I suspect it's a bit faster to compile as Concepts are easier than SFINAE to evaluate. Niall -- ned Productions Limited Consulting http://www.nedproductions.biz/ http://ie.linkedin.com/in/nialldouglas/
On 27 January 2018 at 18:24, Niall Douglas via Boost
My main development environment is MSVC on Windows as it's the lowest common denominator usually.
My IDE is VS2017 as well, but Clang/LLVM is doing the job...
Outcome may cause GCC to sometimes ICE in its unit tests...
Doesn't matter where or how it ICE-es... it simply shouldn't... Your code might crash, due to logic errors, UB, double free etc... but the compiler shouldn't ICE... I'm just stating (in your defense actually) that for now we don't know who the culprit is, but it starts with gcc. If you want more useful compiler error messages, I would recommend wholeheartedly Clang/LLVM https://llvm.org/builds/... That installer does not integrate with VS2017 (it does with VS2015 and lower), but it's trivial to do it manually, I can send you the props and target files (and where to put them) if you want... degski
On 27 January 2018 at 18:40, degski
If you want more useful compiler error messages, I would recommend wholeheartedly Clang/LLVM https://llvm.org/builds/...
From what I read, STL stated that MS is testing the VC-STL against Clang/LLVM to sort out the bugs...
degski
If you want more useful compiler error messages, I would recommend wholeheartedly Clang/LLVM https://llvm.org/builds/...
From what I read, STL stated that MS is testing the VC-STL against Clang/LLVM to sort out the bugs...
The Dinkumware STL is, and always has been, portable across compilers and architectures. QNX ship Dinkumware as the system STL with GCC as the compiler for example. Anyone can buy in Dinkumware as their STL for their whatever weird platform for the right fee. It's a commercial product totally separate from Microsoft, who are merely one of many customers, though probably their biggest. Niall -- ned Productions Limited Consulting http://www.nedproductions.biz/ http://ie.linkedin.com/in/nialldouglas/
If you want more useful compiler error messages, I would recommend wholeheartedly Clang/LLVM https://llvm.org/builds/... That installer does not integrate with VS2017 (it does with VS2015 and lower), but it's trivial to do it manually, I can send you the props and target files (and where to put them) if you want...
Already running it many years now :). Also, Windows Subsystem for Linux using the same source tree as Windows. Works lovely. I also have a personally hacked and recompiled clang-format plugin for Visual Studio which auto-formats on save. Very, very useful. Modern tooling is amazing. I only just recently was having a discussion with Rob Stewart about the merits of intentionally typing out crap unformatted C++ and letting clang-tidy -fix rewrite it for me into non-crap C++, and clang-format to reformat it for me. He was not convinced, but I think he is watching that space. Niall -- ned Productions Limited Consulting http://www.nedproductions.biz/ http://ie.linkedin.com/in/nialldouglas/
On 28/01/2018 00:07, Glen Fernandes via Boost wrote:
On Sat, Jan 27, 2018 at 7:01 PM, degski via Boost wrote:
Any ICE is a compiler issue... so this is the wrong place to report the (ICE-)issue...
Sure. The author putting a library forward for review should feel free to report any compiler defects to vendors that his library exposes. It looks like Niall has done that already.
Yep, clang, MSVC and GCC have all fixed compiler bugs found by Outcome. It's been a good test of their compiler's C++ 14 conformance. clang and GCC have also fixed the code optimisation problems I reported, and Microsoft have told me it's a high priority for MSVC once their new optimiser is debugged fully.
Or are you suggesting that reviewers trying to review the submission during the review (by, among other things, running tests) shouldn't mention to the author that they encountered an ICE?
Oh no, it's definitely important to report ICEs in the compiler. Just please don't judge Outcome harshly for it, just six months ago Outcome was unusable on anything but clang. It's thanks to the compiler vendors being so great with me that Outcome is now widely usable. Niall -- ned Productions Limited Consulting http://www.nedproductions.biz/ http://ie.linkedin.com/in/nialldouglas/
https://travis-ci.org/ned14/boost-outcome
The first one is for boost-outcome, correct? It only tests g++-6 with c++14, not g++-7 (in 14 or 17) (where the ICE occurs), or or clang+libc++ (where your other errors were).
The same constexpr ICE problem afflicts GCC 6. But similarly it happens randomly across machines.
Aren't the second and third for standalone outcome? That isn't being reviewed here, correct? (i.e. Standalone outcome doens't even ICE MSVC2017 with Vinnie's test case, but boost-outcome, the one being submitted for review, does?).
I didn't think it necessary to run more CIs than at present. I know they appear to be free to us, but I felt it anti-social to be using more jobs than is strictly necessary. And the existing CIs cover all of MSVC, clang and GCC, with Dinkumware, libstdc++ and libc++ on Linux, Windows and MacOS. That's a good cross section of the most common configurations. I am surprised about the clang + libc++ missing header because the XCode 9 CI job ought to have caught that, but that happens sometimes. boost-outcome is the same library as Outcome standalone, if you do a side by side diff you'll see that. The only change is the use of Boost.Config instead of SD6 feature macros, and Boost.Exception. Hence I felt that boost-outcome didn't need much testing above making sure it works inside Boost, which is what the CI job does. Niall -- ned Productions Limited Consulting http://www.nedproductions.biz/ http://ie.linkedin.com/in/nialldouglas/
participants (3)
-
degski -
Glen Fernandes -
Niall Douglas