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

rvalue reference, perfect forwarding and variadic template support #6

Merged
merged 2 commits into from Oct 8, 2018

Conversation

Projects
None yet
4 participants
@VemundH
Copy link
Contributor

VemundH commented Sep 7, 2014

Adds rvalue, perfect forwarding and variadic templates.
Variadic templates replaces boost.preprocessor (only for compilers with both rvalue and variadic templates support). This removes the max argument limitation (BOOST_ASSIGN_MAX_PARAMS).

A change was required in assign_decay (related to this boost::decay issue: https://svn.boost.org/trac/boost/ticket/7760).

@VemundH VemundH changed the title Feature/c++11 support rvalue reference, perfect forwarding and variadic template support Sep 7, 2014

@jeking3

This comment has been minimized.

Copy link
Collaborator

jeking3 commented Jul 27, 2018

If you could rebase this on the latest develop and resolve the merge conflict, then we'll get a CI build out of it. Thanks.

@jeking3
Copy link
Collaborator

jeking3 left a comment

I see no tests added as part of these changes. Do you believe the standard set of tests using C++03 and C++11 modes with the pre-existing test code is sufficient? We'll see, once you rebase on master and get a CI build with code coverage.

@VemundH VemundH force-pushed the VemundH:feature/c++11-support branch from 7fc1b97 to 4a51439 Jul 30, 2018

@jeking3
Copy link
Collaborator

jeking3 left a comment

clang-5.0 cxxstd=11 fails, see:
https://api.travis-ci.org/v3/job/409990297/log.txt

@VemundH VemundH force-pushed the VemundH:feature/c++11-support branch from 6477743 to abe71f8 Aug 3, 2018

@codecov

This comment has been minimized.

Copy link

codecov bot commented Aug 3, 2018

Codecov Report

Merging #6 into develop will increase coverage by 2.08%.
The diff coverage is 61.11%.

Impacted file tree graph

@@             Coverage Diff             @@
##           develop       #6      +/-   ##
===========================================
+ Coverage    76.53%   78.62%   +2.08%     
===========================================
  Files           13       13              
  Lines          260      262       +2     
  Branches        55       55              
===========================================
+ Hits           199      206       +7     
+ Misses          10        5       -5     
  Partials        51       51
Impacted Files Coverage Δ
include/boost/assign/std/queue.hpp 50% <ø> (ø) ⬆️
include/boost/assign/std/stack.hpp 50% <ø> (ø) ⬆️
include/boost/assign/ptr_map_inserter.hpp 80% <ø> (ø) ⬆️
include/boost/assign/list_inserter.hpp 87.14% <ø> (ø) ⬆️
include/boost/assign/std/list.hpp 50% <ø> (ø) ⬆️
include/boost/assign/std/deque.hpp 50% <ø> (ø) ⬆️
include/boost/assign/std/set.hpp 50% <ø> (ø) ⬆️
include/boost/assign/std/vector.hpp 50% <ø> (ø) ⬆️
include/boost/assign/ptr_list_inserter.hpp 72.72% <ø> (ø) ⬆️
include/boost/assign/assignment_exception.hpp 66.66% <0%> (+66.66%) ⬆️
... and 3 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 308b6a4...62c23a9. Read the comment docs.

@VemundH VemundH force-pushed the VemundH:feature/c++11-support branch from abe71f8 to 89b0a76 Aug 3, 2018

@VemundH

This comment has been minimized.

Copy link
Contributor

VemundH commented Aug 4, 2018

A long time since I created this pull request.
Added tests for std::array support (the only new feature). The rest is alternative code paths for compilers with C++11 support, and the standard set of tests should be sufficient. I also believe the recently disabled tests on C++11 can be re-enabled with this.

@jeking3 jeking3 requested a review from pdimov Aug 4, 2018

@jeking3 jeking3 requested review from mclow and eldiener and removed request for pdimov Aug 21, 2018

@jeking3

This comment has been minimized.

Copy link
Collaborator

jeking3 commented Aug 21, 2018

The build failure is erroneous - everything passed.

@jeking3 jeking3 merged commit c66a112 into boostorg:develop Oct 8, 2018

3 of 4 checks passed

codecov/patch 61.11% of diff hit (target 76.53%)
Details
codecov/project 78.62% (+2.08%) compared to 308b6a4
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@morinmorin

This comment has been minimized.

Copy link
Member

morinmorin commented Oct 13, 2018

This PR breaks old GCC (<= 4.6) in C++11 mode. For example,
this simple code fails to compile

#include <boost/assign.hpp>

I don't think it's sensible to use C++11 with old GCC, but IMHO "just #including <boost/assign.hpp> makes compilations fail" would be a severe breakage. A fix is to change

`#if defined(BOOST_NO_CXX11_VARIADIC_TEMPLATES) || defined(BOOST_NO_CXX11_RVALUE_REFERENCES)

to

#if defined(BOOST_NO_CXX11_VARIADIC_TEMPLATES) || defined(BOOST_NO_CXX11_RVALUE_REFERENCES) \
 || BOOST_WORKAROUND(BOOST_GCC, < 40700)
@morinmorin

This comment has been minimized.

Copy link
Member

morinmorin commented Oct 13, 2018

This branch (L506 and L670)

#elif !defined(BOOST_NO_CXX11_HDR_INITIALIZER_LIST) && !defined(BOOST_NO_CXX11_FUNCTION_TEMPLATE_DEFAULT_ARGS)
        template< class Container,
            class = typename boost::disable_if< is_initializer_list<Container> >::type
        >
        operator Container() const

needs to SFINAE out Allocator (e.g. by adding template parameter typename Dummy = typename Container::iterator) to fix Trac-7364.

@morinmorin

This comment has been minimized.

Copy link
Member

morinmorin commented Oct 13, 2018

Forgot to say the above comments are about <boost/assign/list_of.hpp>.

@pdimov

This comment has been minimized.

Copy link
Contributor

pdimov commented Oct 13, 2018

This PR breaks old GCC (<= 4.6) in C++11 mode.

Right.

@jeking3, consider expanding the platform coverage in your Travis scripts, as already mentioned in boostorg/function#21. For legacy libraries, you really need 4.4 and 4.6 jobs to catch pull requests that break them. In addition, for g++ 6 and above you want to test C++14 because that's their default and that's what everyone would be using. Your testing g++7 in 03/11 mode is decidedly suboptimal.

The minimum coverage I like is f.ex. https://github.com/boostorg/parameter/blob/develop/.travis.yml - g++ 4.4, 4.6, 4.8 (default), 5, 6, 7, 8; clang with and without libc++, Mac. Some of these can in principle be dropped (you could f.ex. test g++5/6 with 11/14, 7 and 8 with 14/17) but the time saved is usually not worth the loss in coverage, unless the library tests take a really long time.

For community-maintained libraries not breaking things that used to work is a priority.

@pdimov

This comment has been minimized.

Copy link
Contributor

pdimov commented Oct 13, 2018

(And for g++ 7 and above, and for later libc++, you want to test C++17 because of standard library removals that may need addressing.)

@jeking3

This comment has been minimized.

Copy link
Collaborator

jeking3 commented Oct 13, 2018

I'd like the CI builds not to be a full matrix test, because of a couple reasons:

  1. We already have a matrix test, albeit that it catches things after the fact.
  2. Our build backlog is getting really long - I submitted some things last night and they are still waiting after 12 hours. Nobody purchased more slots on Travis to fix this issue.

On the mailing list I thought we had discussed not testing < gcc 4.8 any more, and that folks can happily use older versions of boost if they need to support older compilers or language levels. At some point the compatibility matrix is not sustainable. Any commercial entity would have dropped C++03 support on this project a long time ago due to maintenance costs.

I think there is a balance that needs to be made. Each PR cycle cannot kick off 50 builds which take 10 minutes each.

Were these failures identified in the matrix tests? How does one compare two matrix tests on a repo from different dates to see what the regressions are? Is there a tool that can do this, or is it always manual?

These issues found need to be opened as new issues or as PRs. Commenting here has a chance of getting lost because the PR is merged.

@pdimov

This comment has been minimized.

Copy link
Contributor

pdimov commented Oct 13, 2018

If you would rather break things that worked before, you are not a good fit for the CMT. This is not what the CMT is about.

@jeking3

This comment has been minimized.

Copy link
Collaborator

jeking3 commented Oct 13, 2018

That's not my intention, however the CMT or any other group cannot reasonably support an ever-growing backwards compatibility matrix. There do have to be reasonable limits. I will look into beefing up the CI test matrix in the CMT repositories.

@jeking3

This comment has been minimized.

Copy link
Collaborator

jeking3 commented Oct 13, 2018

@morinmorin isn't that what #20 was for?

@morinmorin

This comment has been minimized.

Copy link
Member

morinmorin commented Oct 13, 2018

@morinmorin isn't that what #20 was for?

Yes, that patch contains a fix for issues with Allocator. But, since a huge patch (i.e. this PR) got merged, I'm pretty sure PR #20 will not be merged cleanly. So, I recommended throwing out #20 and pinpoint how to fix the new code.

@jeking3

This comment has been minimized.

Copy link
Collaborator

jeking3 commented Oct 14, 2018

@pdimov check this out - it catches the issues described:

https://travis-ci.org/jeking3/assign/builds/441266324

I'd like to make the Windows build better as well.

@pdimov

This comment has been minimized.

Copy link
Contributor

pdimov commented Oct 14, 2018

Looks good, you have to use 98 for g++ 4.4 though, it doesn't yet have 03. :-)

@pdimov

This comment has been minimized.

Copy link
Contributor

pdimov commented Oct 14, 2018

If you switch UBSAN to g++ 8, you'll cover that as well without spending another job.

All the clangs seem to be using libstdc++ though. You need to have at least one on libc++, whether by adding a Mac job, or by using libc++ on one of the existing Linux ones.

@jeking3

This comment has been minimized.

Copy link
Collaborator

jeking3 commented Oct 14, 2018

There are even more issues, like many of the clang jobs are still using clang-5. Still working on it... :)

@pdimov

This comment has been minimized.

Copy link
Contributor

pdimov commented Oct 14, 2018

clang-5 is the default clang++ on Travis at the moment, so that's probably why.

For a minimal matrix, having libc++ is more important for coverage than having many clang versions, so I usually go with one default clang and one clang++-libc++. It's not that earlier clangs don't have their oddities but most libraries don't encounter them.

I sometimes add clang-3.4 because that's the default system clang on Trusty: https://github.com/boostorg/smart_ptr/blob/develop/.travis.yml#L130

but it's really not that critical.

@jeking3

This comment has been minimized.

Copy link
Collaborator

jeking3 commented Oct 14, 2018

@morinmorin is the detection logic for the macro BOOST_NO_CXX11_VARIADIC_TEMPLATES correct for gcc 4.4 and 4.6?

@jeking3

This comment has been minimized.

Copy link
Collaborator

jeking3 commented Oct 14, 2018

@pdimov have you seen anything like this before on osx jobs? Simple commands like "cd" are failing.

$ source ci/travis/install.sh
++++basename /Users/travis/build/jeking3/assign
+++export SELF=assign
+++SELF=assign
+++cd ..
+++__zsh_like_cd cd ..
+++typeset __zsh_like_cd_hook
+++builtin cd ..
(it just dies a few lines after that)

https://travis-ci.org/jeking3/assign/jobs/441316951

@morinmorin

This comment has been minimized.

Copy link
Member

morinmorin commented Oct 14, 2018

@morinmorin is the detection logic for the macro BOOST_NO_CXX11_VARIADIC_TEMPLATES correct for gcc 4.4 and 4.6?

In some rare cases (mainly for oldish compilers), Boost.Config says FEATURE_XXX is implemented in the compiler even if the implementation is incomplete. This is the case for the variadic template feature on GCC 4.4–4.6.
Well, both GCC 4.4–4.6 have basic functionalities of variadic templates and so they pass the test in Boost.Config.

For advanced usage like our case, we should use

#if defined(BOOST_NO_CXX11_VARIADIC_TEMPLATES) || BOOST_WORKAROUND(BOOST_GCC, < 40700)

to detect unsupported compilers.

@jeking3

This comment has been minimized.

Copy link
Collaborator

jeking3 commented Oct 14, 2018

@morinmorin Is there just one particular block that needs this treatment, or other files in this repo?

jking@ubuntu:~/boost/libs/assign$ grep -r VARIADIC_TEMPLATES .
./include/boost/assign/ptr_list_of.hpp:#if defined(BOOST_NO_CXX11_VARIADIC_TEMPLATES) || defined(BOOST_NO_CXX11_RVALUE_REFERENCES)
./include/boost/assign/ptr_list_of.hpp:#if defined(BOOST_NO_CXX11_VARIADIC_TEMPLATES) || defined(BOOST_NO_CXX11_RVALUE_REFERENCES)
./include/boost/assign/ptr_list_of.hpp:#if defined(BOOST_NO_CXX11_VARIADIC_TEMPLATES) || defined(BOOST_NO_CXX11_RVALUE_REFERENCES)
./include/boost/assign/ptr_list_of.hpp:#if defined(BOOST_NO_CXX11_VARIADIC_TEMPLATES) || defined(BOOST_NO_CXX11_RVALUE_REFERENCES)
./include/boost/assign/ptr_map_inserter.hpp:#if defined(BOOST_NO_CXX11_VARIADIC_TEMPLATES) || defined(BOOST_NO_CXX11_RVALUE_REFERENCES)
./include/boost/assign/ptr_map_inserter.hpp:#if defined(BOOST_NO_CXX11_VARIADIC_TEMPLATES) || defined(BOOST_NO_CXX11_RVALUE_REFERENCES)
./include/boost/assign/ptr_map_inserter.hpp:#if defined(BOOST_NO_CXX11_VARIADIC_TEMPLATES) || defined(BOOST_NO_CXX11_RVALUE_REFERENCES)
./include/boost/assign/list_of.hpp:#if defined(BOOST_NO_CXX11_VARIADIC_TEMPLATES) || defined(BOOST_NO_CXX11_RVALUE_REFERENCES)
./include/boost/assign/list_of.hpp:#if defined(BOOST_NO_CXX11_VARIADIC_TEMPLATES) || defined(BOOST_NO_CXX11_RVALUE_REFERENCES)
./include/boost/assign/list_of.hpp:#if defined(BOOST_NO_CXX11_VARIADIC_TEMPLATES) || defined(BOOST_NO_CXX11_RVALUE_REFERENCES)
./include/boost/assign/list_of.hpp:#if defined(BOOST_NO_CXX11_VARIADIC_TEMPLATES) || defined(BOOST_NO_CXX11_RVALUE_REFERENCES)
./include/boost/assign/list_of.hpp:#if defined(BOOST_NO_CXX11_VARIADIC_TEMPLATES) || defined(BOOST_NO_CXX11_RVALUE_REFERENCES)
./include/boost/assign/list_inserter.hpp:#if defined(BOOST_NO_CXX11_VARIADIC_TEMPLATES) || defined(BOOST_NO_CXX11_RVALUE_REFERENCES)
./include/boost/assign/list_inserter.hpp:#if defined(BOOST_NO_CXX11_VARIADIC_TEMPLATES)
./include/boost/assign/list_inserter.hpp:#if defined(BOOST_NO_CXX11_VARIADIC_TEMPLATES) || defined(BOOST_NO_CXX11_RVALUE_REFERENCES)
./include/boost/assign/list_inserter.hpp:#if defined(BOOST_NO_CXX11_VARIADIC_TEMPLATES)
./include/boost/assign/list_inserter.hpp:#if defined(BOOST_NO_CXX11_VARIADIC_TEMPLATES) || defined(BOOST_NO_CXX11_RVALUE_REFERENCES)
./include/boost/assign/ptr_list_inserter.hpp:#if defined(BOOST_NO_CXX11_VARIADIC_TEMPLATES) || defined(BOOST_NO_CXX11_RVALUE_REFERENCES)
./include/boost/assign/ptr_list_inserter.hpp:#if defined(BOOST_NO_CXX11_VARIADIC_TEMPLATES) || defined(BOOST_NO_CXX11_RVALUE_REFERENCES)
./include/boost/assign/ptr_list_inserter.hpp:#if defined(BOOST_NO_CXX11_VARIADIC_TEMPLATES) || defined(BOOST_NO_CXX11_RVALUE_REFERENCES)
@morinmorin

This comment has been minimized.

Copy link
Member

morinmorin commented Oct 14, 2018

@morinmorin Is there just one particular block that needs this treatment, or other files in this repo?

Fixing <boost/assign/list_of.hpp> makes this code

#include <boost/assign.hpp>

compile fine. So I guess just one file needs the treatment (though I haven't looked into other files).

That said, it might be better to define

#if !(defined(BOOST_NO_CXX11_VARIADIC_TEMPLATES) || BOOST_WORKAROUND(BOOST_GCC, < 40700))
// or #if !(defined(BOOST_NO_CXX11_VARIADIC_TEMPLATES) || BOOST_WORKAROUND(BOOST_GCC, < 40700) \
//       || defined(BOOST_NO_CXX11_RVALUE_REFERENCES))
#define BOOST_ASSIGN_USE_VARIADIC_TEMPLATES

and use that macro extensively in Boost.Assign.

@pdimov

This comment has been minimized.

Copy link
Contributor

pdimov commented Oct 14, 2018

This change broke the Range tests by the way. https://travis-ci.org/boostorg/range/builds/441323699

@jeking3

This comment has been minimized.

Copy link
Collaborator

jeking3 commented Oct 14, 2018

@morinmorin

This comment has been minimized.

Copy link
Member

morinmorin commented Oct 15, 2018

Wow, this PR broke the following code on all platforms!

#include <boost/assign/list_of.hpp>

Don't worry, this is simply fixed by adding

#include <boost/config.hpp>

to <boost/assign/assignment_exception.hpp> ;)

@jeking3

This comment has been minimized.

Copy link
Collaborator

jeking3 commented Oct 15, 2018

@morinmorin I included that in #27

@pdimov

This comment has been minimized.

Copy link
Contributor

pdimov commented Oct 15, 2018

The tests should be fixed to include the Assign header first, "per best practices."

By the look of it, they did start out this way, but then someone added the detail/workaround.hpp include in front, defeating the purpose.

@jeking3 jeking3 referenced this pull request Oct 16, 2018

Closed

MSVC 2012 build error #15

@jeking3 jeking3 referenced this pull request Nov 3, 2018

Closed

Release notes for 1.69.0 #23

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment