C++17 prep - namespace qualifiers only

[andrjohns]

Summary

This PR implements a subset of the changes from #2641 to assess whether the test leak-sanitizers errors are still present.

This PR implements only the namespace qualification of apply and size, leaving the Eigen::Index and constexpr changes for a separate PR

Tests

N/A - Current tests should still pass

Side Effects

C++17 compatibility

Release notes

Added namespace qualifiers to size and apply calls for c++17 compatibility

Checklist

• Math issue Update to Eigen 3.4 #2474

• Copyright holder: Andrew Johnson

  The copyright holder is typically you or your assignee, such as a university or company. By submitting this pull request, the copyright holder is agreeing to the license the submitted work under the following licenses:
  - Code: BSD 3-clause (https://opensource.org/licenses/BSD-3-Clause)
  - Documentation: CC-BY 4.0 (https://creativecommons.org/licenses/by/4.0/)

• the basic tests are passing

  • unit tests pass (to run, use: ./runTests.py test/unit)
  • header checks pass, (make test-headers)
  • dependencies checks pass, (make test-math-dependencies)
  • docs build, (make doxygen)
  • code passes the built in C++ standards checks (make cpplint)

• the code is written in idiomatic C++ and changes are documented in the doxygen

• the new changes are tested

[andrjohns]

It looks like the profiling framework is tickling a bug specific to clang++-6.0 and c++17:

test/unit/math/rev/core/profiling_threading_test --gtest_output="xml:test/unit/math/rev/core/profiling_threading_test.xml"
Running main() from lib/benchmark_1.5.1/googletest/googletest/src/gtest_main.cc
[==========] Running 1 test from 1 test suite.
[----------] Global test environment set-up.
[----------] 1 test from Profiling
[ RUN      ] Profiling.profile_threading
double free or corruption (out)
Aborted (core dumped)

There aren't any issues with reduce_sum, and the tests pass in the current PR when run with std=c++1y, or with clang++-7 & c++17, or with g++ & c++17

Given that the move to c++17 will require upping the minimum g++ compiler, we could also increase the clang minimum from clang-6.0 to clang-7?

[serban-nicusor-toptal]

Given that the move to c++17 will require upping the minimum g++ compiler, we could also increase the clang minimum from clang-6.0 to clang-7?

Will upping g++ and clang affect the other repositories CI functionality? I'm asking this so I know if we should move Math to a separate Docker image or use the same across the CI ( as we're doing now ).
I will open a PR in ci-scripts with updated g++ and clang then build a separate image so we can test and debug in this PR and if everything is fine we can decide to also update the latest tag with it.

Edit: Nvidia has CDN problems and I can't build the image because of the missing Release file in the apt repos, see: NVIDIA/nvidia-docker#1624

Edit: It's now fixed!

[andrjohns]

Great, thanks!

[serban-nicusor-toptal]

Hey @andrjohns you can find the new image build at: stanorg/ci:gpu-cpp17 so you can now reference it this pull request for testing.
There's also a PR open so we can have these changes in the CI after the merge.
You can commit any changes there and let me know if you need a fresh rebuild.

As soon as your testing is done let me know so we can coordinate on updating the main ci tag. Thanks!

[serban-nicusor-toptal]

Hey, I think the current Jenkins failure 3 != 0 will be fixed when #2695 is merged in FYI.

[SteveBronder]

@andrjohns I think you need to import the update to Rtools 4.0 change from the Eigen 3.4 PR to compile with C++17 for some of these tests. imo thats a thing we should just be doing anyway

[andrjohns]

These UNSTABLE failures are a bit odd. The tests are all passing, it's just getting hung up because it can't find one of the Boost headers during clean-up:

[](https://jenkins.flatironinstitute.org/blue/organizations/jenkins/Stan%2FCmdStan/detail/downstream_tests/47/pipeline/115#step-179-log-6)[Linux interface tests with MPI@GCC4] [-ERROR-] Can't create fingerprints for some files:

[Linux interface tests with MPI@GCC4] [-ERROR-] - 'C:/Users/jenkins/workspace/Stan_CmdStan_downstream_tests/stan/lib/stan_math/lib/tbb/stan/lib/stan_math/lib/boost_1.75.0/boost/core/allocator_access.hpp' file not found

Not entirely sure what to make of that

[rok-cesnovar]

I see that Nic restarted the tests - 🤞🏻 that it's a one-off thing. Locally I have no issue building/cleaning up.

@andrjohns is there a plan on how/when we are going to move to C++17. I fully support that move - just wondering.

[serban-nicusor-toptal]

I'm not entirely sure what in this PR makes the build unstable

• this unstable build
• a successful build from another PR

Maybe a cache issue of some sort in Jenkins ? If we're sure it's not because of any change in the PR, we can try to open a new PR with these changes as it will create a fresh build env.

[SteveBronder]

So it looks like this is compiling now! @andrjohns should we just get rid of the CI changes and then merge?

[andrjohns]

@SteveBronder sorry for the delay! I've reverted those CI changes, can you approve when you have a minute?

[serban-nicusor-toptal]

Hey, I think we should leave this PR Jenkinsfile to use image 'stanorg/ci:gpu-cpp17 as we want that moving forward with master, but image 'stanorg/ci:gpu' should still be used for old PRs still Open in a backward compatibility mode.
If we know that old PRs will work with ci:gpu-cpp17 we can push it as the main tag. What do you think ?

[andrjohns]

Sounds good to me! I'll add that back in