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

Liberally scatter BOOST_CXX14_CONSTEXPR throughout #28

Merged
merged 1 commit into from Oct 29, 2018

Conversation

Projects
None yet
3 participants
@tonyelewis
Copy link
Contributor

tonyelewis commented Oct 27, 2018

Add BOOST_CXX14_CONSTEXPR throughout the code.

@tonyelewis

This comment has been minimized.

Copy link
Contributor

tonyelewis commented Oct 27, 2018

This is discussed in #29.

@jeking3

This comment has been minimized.

Copy link
Collaborator

jeking3 commented Oct 27, 2018

CI as it stands right now will not properly test this. Could you pull in the expanded .travis.yml from Boost.Assign? Nothing else needed, just copy the file in from develop tip. That will greatly expand the CI to include many more C++ language levels.

@jeking3
Copy link
Collaborator

jeking3 left a comment

CI needs updating to properly test.

@jeking3

This comment has been minimized.

Copy link
Collaborator

jeking3 commented Oct 28, 2018

test/rational_test.cpp:1296:5:   instantiated from here
../../boost/rational.hpp:178:91: internal compiler error: in build_data_member_initialization, at cp/semantics.c:5553
Please submit a full bug report,
with preprocessed source if appropriate.
See <file:///usr/share/doc/gcc-4.6/README.Bugs> for instructions.
Preprocessed source stored into /tmp/cctRuDKY.out file, please attach this to your bugreport.
    "g++-4.6"   -std=c++0x -fvisibility-inlines-hidden -fPIC -m64 -O3 -finline-functions -Wno-inline -Wall -fvisibility=hidden  -DBOOST_ALL_NO_LIB=1 -DBOOST_CHRONO_STATIC_LINK=1 -DBOOST_CHRONO_THREAD_DISABLED -DBOOST_SYSTEM_STATIC_LINK=1 -DBOOST_TEST_NO_AUTO_LINK=1 -DBOOST_TIMER_STATIC_LINK=1 -DNDEBUG  -I"../.." -c -o "../../bin.v2/libs/rational/test/rational_test.test/gcc-4.6/release/cxxstd-0x-iso/visibility-hidden/rational_test.o" "test/rational_test.cpp"
...failed gcc.compile.c++ ../../bin.v2/libs/rational/test/rational_test.test/gcc-4.6/release/cxxstd-0x-iso/visibility-hidden/rational_test.o...
@tonyelewis

This comment has been minimized.

Copy link
Contributor

tonyelewis commented Oct 28, 2018

So it looks like there was an internal compiler error in GCC 4.6 and problems in the test framework in a UBSan build. Neither of these look like direct consequences of the changes. I've submitted #30 in which I've copied the .travis.yml from Boost Assign to check that these tests were passing without this PR's changes.

@jzmaddock

This comment has been minimized.

Copy link
Contributor

jzmaddock commented Oct 28, 2018

CI failures not withstanding, yes please to more constexpr!

I'm sure there are other C++11/14 features that can be leveraged here (noexcept, moves?)

@tonyelewis

This comment has been minimized.

Copy link
Contributor

tonyelewis commented Oct 29, 2018

From what I can see, PR #32 also experiences the same failures so I don't think the changes in this PR's original commit are making anything worse.

@jeking3

This comment has been minimized.

Copy link
Collaborator

jeking3 commented Oct 29, 2018

Thanks, let's go back to this PR without the expanded CI and I will merge it, and we'll deal with the expanded CI separately.

@tonyelewis

This comment has been minimized.

Copy link
Contributor

tonyelewis commented Oct 29, 2018

OK. Any preference whether I force-remove the last commit or add another commit that reverts it?

@jeking3

This comment has been minimized.

Copy link
Collaborator

jeking3 commented Oct 29, 2018

I would remove it entirely through a rebase, dropping it, and force push.

@codecov

This comment has been minimized.

Copy link

codecov bot commented Oct 29, 2018

Codecov Report

Merging #28 into develop will not change coverage.
The diff coverage is 100%.

Impacted file tree graph

@@           Coverage Diff            @@
##           develop      #28   +/-   ##
========================================
  Coverage    67.52%   67.52%           
========================================
  Files            1        1           
  Lines          271      271           
  Branches        90       90           
========================================
  Hits           183      183           
  Misses           3        3           
  Partials        85       85
Impacted Files Coverage Δ
include/boost/rational.hpp 67.52% <100%> (ø) ⬆️

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 2c7fe1a...8fd02ac. Read the comment docs.

@tonyelewis

This comment has been minimized.

Copy link
Contributor

tonyelewis commented Oct 29, 2018

OK. I've removed the last commit entirely.

@jeking3 jeking3 merged commit ec10199 into boostorg:develop Oct 29, 2018

0 of 2 checks passed

continuous-integration/travis-ci/pr The Travis CI build could not complete due to an error
Details
continuous-integration/appveyor/pr Waiting for AppVeyor build to complete
Details

@tonyelewis tonyelewis deleted the tonyelewis:add-cxx14-constexpr branch Nov 1, 2018

@tonyelewis

This comment has been minimized.

Copy link
Contributor

tonyelewis commented Nov 1, 2018

Thanks. I've just submitted PR #32 that follows up on some of these issues.

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