Replace deprecated std::auto_ptr with std::unique_ptr and move some header files out of src
#9
Conversation
This fixes more compilation erros that I have when building this package on Arch Linux.
|
A new Pull Request was created by @guitargeek (Jonas Rembser) for branch cms/CORAL_2_3_21py3. @cmsbuild, @smuzaffar, @iarspider can you please review it and eventually sign? Thanks. |
|
please test |
|
please test for slc7_amd64_gcc11 |
|
-1 Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-c0f4a3/22404/summary.html External BuildI found compilation error when building: >> Entering Package LCG/MonitoringService Entering library rule at LCG/MonitoringService >> Entering Package LCG/ConnectionService Entering library rule at LCG/ConnectionService + '[' '' == set ']' error: Bad exit status from /data/cmsbld/jenkins/workspace/ib-run-pr-tests/testBuildDir/tmp/rpm-tmp.dJAlPh (%build) RPM build errors: Bad exit status from /data/cmsbld/jenkins/workspace/ib-run-pr-tests/testBuildDir/tmp/rpm-tmp.dJAlPh (%build) |
|
-1 Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-c0f4a3/22405/summary.html External BuildI found compilation error when building: >> Entering Package LCG/MonitoringService Entering library rule at LCG/MonitoringService >> Entering Package LCG/ConnectionService Entering library rule at LCG/ConnectionService + '[' '' == set ']' error: Bad exit status from /data/cmsbld/jenkins/workspace/ib-run-pr-tests/testBuildDir/tmp/rpm-tmp.IjLRyh (%build) RPM build errors: line 37: It's not recommended to have unversioned Obsoletes: Obsoletes: cms+coral+CORAL_2_3_21-e505304864e8a0af44b6b70e2d1fd01a Bad exit status from /data/cmsbld/jenkins/workspace/ib-run-pr-tests/testBuildDir/tmp/rpm-tmp.IjLRyh (%build) |
|
Hi, sorry I can't login to see what the actual error is. Can you please copy paste the CI compiler error here so hopefully I can find a easy fix? Thanks! |
|
|
Pull request #9 was updated. |
|
Thanks for letting me know! I see now that I missed another |
|
please test |
|
+1 Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-c0f4a3/22425/summary.html The following merge commits were also included on top of IB + this PR after doing git cms-merge-topic:
You can see more details here: Comparison SummarySummary:
|
|
Cool, it worked! What about testing also with |
|
As a PR author you can start the tests yourself :) |
|
please test for slc7_amd64_gcc11 |
Ah ok, thanks I didn't know! |
|
please test for slc7_ppc64le_gcc11 |
|
|
||
| return *this; | ||
| } | ||
|
|
There was a problem hiding this comment.
@guitargeek why to remove this code?
There was a problem hiding this comment.
That code was apparently written before move symanics were in place. The copy constructor for auto_ptr here acts like the move constructor for the unique_ptr [1]. This means that the right record is now invalid bevause it's m_data was reset to nullptr. A copy constructor where the other object is mutated and then invalid is dangerous, we have move constructors for that now (and the move constructor is even generated by default for us, that's why code is removed but not added).
[1] https://en.cppreference.com/w/cpp/memory/auto_ptr/operator%3D
| @@ -22,31 +22,6 @@ namespace coral | |||
| { | |||
There was a problem hiding this comment.
@guitargeek , shouldn;t we have m_data( std::move(data)) here?
There was a problem hiding this comment.
The consructor takes a raw pointer Payload* data that is passed to the constructor of std::unique_ptr<Payload> m_data. Raw pointers don't need to me moved. If the data passed the the Record constructor would be a smart pointer already, you would be right though.
CoralBase/cmt/requirements
Outdated
| @@ -32,7 +32,7 @@ macro_append use_cppflags '' target-vc9 ' /D_CRT_SECURE_NO_WARNINGS' | |||
| macro_append use_cppflags '' target-vc9 ' /D_SCL_SECURE_NO_WARNINGS' | |||
|
|
|||
| # Test -std=c++0x for gcc 4.5.1 as in CMS (see task #18131) | |||
| # Disable "auto_ptr is deprecated" warning for gcc 4.5. | |||
| # Disable "unique_ptr is deprecated" warning for gcc 4.5. | |||
There was a problem hiding this comment.
this change was not required. The comment here is related to gcc 4.5 so I think the old comment makes better sense
There was a problem hiding this comment.
Oh, yes thanks for catching this! I'll update.
This is to avoid getting flooded with deprecation warnings when compiling this project.
|
Pull request #9 was updated. |
|
please test |
|
thanks @guitargeek , most of these changes do not effect CMSSW as we do not build any coral tests e.g. files under and these look good. Once PR tests here are successfull then we will first tests it in cmssw DEVEL IBs. |
|
please test for slc7_ppc64le_gcc11 |
|
-1 Failed Tests: Build The following merge commits were also included on top of IB + this PR after doing git cms-merge-topic: You can see more details here: BuildI found compilation error when building: >> Compiling /scratch/cmsbuild/jenkins_a/workspace/ib-run-pr-tests/CMSSW_12_3_X_2022-02-20-2300/src/RecoTracker/MkFitCore/src/Matriplex/MatriplexCommon.cc
In file included from /scratch/cmsbuild/jenkins_a/workspace/ib-run-pr-tests/CMSSW_12_3_X_2022-02-20-2300/src/RecoTracker/MkFitCore/src/Matriplex/MatriplexSym.h:4,
from /scratch/cmsbuild/jenkins_a/workspace/ib-run-pr-tests/CMSSW_12_3_X_2022-02-20-2300/src/RecoTracker/MkFitCore/src/Matrix.h:27,
from /scratch/cmsbuild/jenkins_a/workspace/ib-run-pr-tests/CMSSW_12_3_X_2022-02-20-2300/src/RecoTracker/MkFitCore/src/KalmanUtilsMPlex.h:5,
from /scratch/cmsbuild/jenkins_a/workspace/ib-run-pr-tests/CMSSW_12_3_X_2022-02-20-2300/src/RecoTracker/MkFitCore/src/KalmanUtilsMPlex.cc:1:
/scratch/cmsbuild/jenkins_a/workspace/ib-run-pr-tests/CMSSW_12_3_X_2022-02-20-2300/src/RecoTracker/MkFitCore/src/Matriplex/MatriplexCommon.h:13:10: fatal error: immintrin.h: No such file or directory
13 | #include "immintrin.h"
| ^~~~~~~~~~~~~
compilation terminated.
In file included from /scratch/cmsbuild/jenkins_a/workspace/ib-run-pr-tests/CMSSW_12_3_X_2022-02-20-2300/src/RecoTracker/MkFitCore/src/Matriplex/MatriplexSym.h:4,
from /scratch/cmsbuild/jenkins_a/workspace/ib-run-pr-tests/CMSSW_12_3_X_2022-02-20-2300/src/RecoTracker/MkFitCore/src/Matrix.h:27,
|
|
+1 Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-c0f4a3/22548/summary.html The following merge commits were also included on top of IB + this PR after doing git cms-merge-topic: You can see more details here: Comparison SummarySummary:
|
|
please test for slc7_ppc64le_gcc11 |
|
-1 Failed Tests: UnitTests The following merge commits were also included on top of IB + this PR after doing git cms-merge-topic:
You can see more details here: Unit TestsI found errors in the following unit tests: ---> test DRNTest had ERRORS |
|
+externals |
|
This pull request is fully signed and it will be integrated in one of the next cms/CORAL_2_3_21py3 IBs (tests are also fine). This pull request will now be reviewed by the release team before it's merged. @perrotta, @dpiparo, @qliphy (and backports should be raised in the release meeting by the corresponding L2) |
CORAL: Replace deprecated std::auto_ptr with std::unique_ptr and move some header files out of src (cms-externals/coral#9)
See the commit messages for more details. Now everything compiles without warnings and errors for me, so no further PRs are expected.
And sorry, I accidentally triggered GitHub to close #8 for good by pushing the wrong branch! This is the same PR as before, but with also the commit moving the header files and with a fix that should resolve the test problems in the previous PR, where a
std::movewas missing.