Replace swap_or_assign with move in edm::Wrapper<T> constructor #22069

makortel · 2018-02-01T10:32:51Z

This PR suggests to replace swap_or_assign() with move assignment in the constructor of edm::Wrapper<T> in order to benefit from compiler-generated move constructor+assignment operator for move-only products instead of having to explicitly write the swap() method every time (I got annoyed and after consultation with @Dr15Jones proceeded for a PR). Here the swap() was anyway used for a "move" before C++11.

I had to add the move constructor and assignment operators to handful of classes because they were not generated automatically by the compiler (mostly because of deleted copy constructor+assignment operator). There were a few more complex cases:

edmNew::DetSetVectorTrans needed explicit move constructor+assignment operator because of atomic member variable (which are not movable), logic is as in swap()
MeasurementTrackerEvent needed explicit move constructor+assignment operator because of the use of a boolean member variable to denote ownership of pointers
In LHEEventProduct ~~I replaced auto_ptr<PDF> member with unique_ptr<PTR>, after which the compiler-generated move constructor+assignment operator work out of the box~~ needed explicit move constructor+assignment operator because of auto_ptr
- ~~and anyway auto_ptr was deprecated in C++11 and removed in C++17~~

For all touched product classes I removed the swap() as unnecessary, except for edmNew::DetSetVector(Trans) where the swap() is used also elsewhere (and swap() is still part of the std::vector interface so for interface compatibility it makes sense to keep it).

Tested with limited matrix in 10_0_0, no changes expected.

…tails::DetSetVectorTrans! Needed because std::atomic does not have move constructor (or assignment operator).

…tors

…pCache, RegionsSeedingHitSets, IntermediateHit{Doublets,Triplets}

…etCorrector

…Event

…lets} as unnecessary

cmsbuild · 2018-02-01T10:33:10Z

The code-checks are being triggered in jenkins.

cmsbuild · 2018-02-01T10:35:22Z

-code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-22069/3189

Code check has found code style and quality issues which could be resolved by applying a patch in https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-22069/3189/git-diff.patch
e.g. curl https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-22069/3189/git-diff.patch | patch -p1

You can run scram build code-checks to apply code checks directly

cmsbuild · 2018-02-20T18:15:21Z

+1
Tested at: ef15111
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-22069/26189/summary.html

The following merge commits were also included on top of IB + this PR after doing git cms-merge-topic:
6138cf4
You can see more details here:
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-22069/26189/git-log-recent-commits
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-22069/26189/git-merge-result

cmsbuild · 2018-02-20T18:15:24Z

Comparison job queued.

cmsbuild · 2018-02-20T18:57:29Z

Comparison is ready
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-22069/26189/summary.html

Comparison Summary:

No significant changes to the logs found
Reco comparison results: 0 differences found in the comparisons
DQMHistoTests: Total files compared: 28
DQMHistoTests: Total histograms compared: 2498032
DQMHistoTests: Total failures: 1
DQMHistoTests: Total nulls: 0
DQMHistoTests: Total successes: 2497855
DQMHistoTests: Total skipped: 176
DQMHistoTests: Total Missing objects: 0
DQMHistoSizes: Histogram memory added: 1.05999999998 KiB( 23 files compared)
Checked 115 log files, 9 edm output root files, 28 DQM output files

perrotta · 2018-02-20T19:38:40Z

+1

Rather technical code improvent which also touches a few reco classes, with no effect on the reco outputs, as expected
(There are no more differences in jenkins outputs when running together with Fix undefined behaviour in MixCollection<T>::MixItr #22270: this confirms once again that they were not due to this PR - and moreover that the fix coded in Fix undefined behaviour in MixCollection<T>::MixItr #22270 is effective, by the way)

fabiocos · 2018-02-21T08:10:59Z

@civanch @perrozzi @efeyazgan could you please check?

perrozzi · 2018-02-22T10:19:05Z

SimDataFormats/GeneratorProducts/interface/LHEEventProduct.h

@@ -24,6 +24,27 @@ class LHEEventProduct {
 	LHEEventProduct(const lhef::HEPEUP &hepeup,
 			const double originalXWGTUP) : 
 	  hepeup_(hepeup), originalXWGTUP_(originalXWGTUP) {}
+	LHEEventProduct(LHEEventProduct&& other) {


may I ask what are these additions used for?

LHEEventProduct(LHEEventProduct&& other) is a move constructor and LHEEventProduct& operator=(LHEEventProduct&& other) is a move assignment operator. They need to be explicitly defined for LHEEventProduct because of auto_ptr member.

Motivation (from PR description) is to replace the current pre-C++11 "swap-or-copy" idiom inside edm::Wrapper with move in order to not require move-only products to define swap() when compiler-generated move constructor and assignment operator are enough.

perrozzi · 2018-02-22T10:34:54Z

+1

civanch · 2018-02-22T16:10:53Z

+1

fabiocos · 2018-02-23T16:50:08Z

+1

fabiocos · 2018-02-23T16:50:40Z

merge

SanghyunKo · 2018-03-17T03:59:22Z

SimDataFormats/GeneratorProducts/interface/LHEEventProduct.h

+	  originalXWGTUP_ = std::move(originalXWGTUP_);
+	  scales_ = std::move(scales_);
+	  npLO_ = std::move(npLO_);
+	  npNLO_ = std::move(npNLO_);


Hi @makortel , I'm MC generator validator of PdmV group. While validating 10_1_0_pre2, I've found that some informations such as npLO & npNLO are missing in generated MC samples. It seems that a bug is introduced from this PR, especially SimDataFormats package. I tried adding other. in L32-35 & L42-45, but simply adding it could not fix the issue. May I ask you to review this PR? Any questions would be appreciated.

@SanghyunKo Whoops, that's certainly a bug (sorry about that). But I'm puzzled why adding other. on the right-hand sides of lines 32-35 and 42-45 would not fix the issue. Could you give me a (simple) recipe how to check myself whether the bug is fixed or not?

@SanghyunKo
I see the other was already added by #22429 (merged in 10_1_0_pre3). I'm nevertheless interested to double-check if the fix actually works or not, so

Could you give me a (simple) recipe how to check myself whether the bug is fixed or not?

@makortel Here I attached link to dropbox that contains simple command that I used to test it, python configuration file and some example of generated LHE file in pre1 & pre2 respectively. I'm also puzzled but let's cross-check it first.

Link to dropbox

@makortel Sorry, I should have checked pre3 before reporting this. I've just checked it with pre3 and it seems that somehow this issue is fixed in pre3.

Also I've just confirmed that adding other. fixed the issue in pre2. Maybe I did something wrong while compiling it previously. Anyway, thanks for the reference to #22429.

@SanghyunKo
Thanks for letting me know you got it to work as well (I can also confirm it with the test you provided).

makortel added 13 commits January 31, 2018 11:18

Replace use of swap_or_assign with std::move

fd30d5e

Add custom move constructor and assignment operator to edmNew::dstvde…

fdcfef1

…tails::DetSetVectorTrans! Needed because std::atomic does not have move constructor (or assignment operator).

Replace auto_ptr with unique_ptr in LHEEventProduct

309cbcf

Add explicit move constructor and assignment operator to LHEEventProduct

38e9575

Add move constructors and assignment operators to various sim associa…

a6a4cf7

…tors

Add explicit move constructors and assignment operators to LayerHitMa…

8c424c1

…pCache, RegionsSeedingHitSets, IntermediateHit{Doublets,Triplets}

Move move assignment operator to public and add move constructor in J…

7e5588e

…etCorrector

Enable move constructor and assignment operator in MeasurementTracker…

f7e0808

…Event

Remove swap_or_assign and infrastructure behind it as obsolete

a7cef2a

Remove swap from JetCorrector as unnecessary

6b2bc5e

Remove swap from RegionsSeedingHitSets, IntermediateHit{Doublets,Trip…

82aaa58

…lets} as unnecessary

Remove swap from MeasurementTrackerEvent as unnecessary

e919683

Remove swap from sim associators as unnecessary

f7ed59e

cmsbuild added this to the CMSSW_10_1_X milestone Feb 1, 2018

cmsbuild added analysis-pending code-checks-pending comparison-pending core-pending generators-pending orp-pending pending-signatures reconstruction-pending simulation-pending tests-pending labels Feb 1, 2018

cmsbuild added code-checks-rejected and removed code-checks-pending labels Feb 1, 2018

Fix

6f92568

cmsbuild removed the code-checks-rejected label Feb 1, 2018

cmsbuild added comparison-pending tests-pending tests-started and removed tests-pending labels Feb 20, 2018

cmsbuild added tests-approved and removed tests-started labels Feb 20, 2018

cmsbuild added comparison-available and removed comparison-pending labels Feb 20, 2018

cmsbuild added reconstruction-approved and removed reconstruction-pending labels Feb 20, 2018

perrozzi reviewed Feb 22, 2018

View reviewed changes

cmsbuild added generators-approved and removed generators-pending labels Feb 22, 2018

cmsbuild added simulation-approved and removed simulation-pending labels Feb 22, 2018

cmsbuild added orp-approved and removed orp-pending labels Feb 23, 2018

cmsbuild merged commit 740adde into cms-sw:master Feb 23, 2018

SanghyunKo reviewed Mar 17, 2018

View reviewed changes

makortel mentioned this pull request Apr 18, 2018

Add move constructor and assignment operator to edm::HepMCProduct #22989

Merged

fabiocos mentioned this pull request Jan 25, 2019

Crash when reading new LS in LHE dataset and local file #25708

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Replace swap_or_assign with move in edm::Wrapper<T> constructor #22069

Replace swap_or_assign with move in edm::Wrapper<T> constructor #22069

makortel commented Feb 1, 2018 •

edited

cmsbuild commented Feb 1, 2018

cmsbuild commented Feb 1, 2018

cmsbuild commented Feb 20, 2018

cmsbuild commented Feb 20, 2018

cmsbuild commented Feb 20, 2018

perrotta commented Feb 20, 2018

fabiocos commented Feb 21, 2018

perrozzi Feb 22, 2018 •

edited

makortel Feb 22, 2018

perrozzi commented Feb 22, 2018

civanch commented Feb 22, 2018

fabiocos commented Feb 23, 2018

fabiocos commented Feb 23, 2018

SanghyunKo Mar 17, 2018

makortel Mar 17, 2018

makortel Mar 17, 2018

SanghyunKo Mar 17, 2018

SanghyunKo Mar 17, 2018 •

edited

makortel Mar 20, 2018

Replace swap_or_assign with move in edm::Wrapper<T> constructor #22069

Replace swap_or_assign with move in edm::Wrapper<T> constructor #22069

Conversation

makortel commented Feb 1, 2018 • edited

cmsbuild commented Feb 1, 2018

cmsbuild commented Feb 1, 2018

cmsbuild commented Feb 20, 2018

cmsbuild commented Feb 20, 2018

cmsbuild commented Feb 20, 2018

perrotta commented Feb 20, 2018

fabiocos commented Feb 21, 2018

perrozzi Feb 22, 2018 • edited

Choose a reason for hiding this comment

makortel Feb 22, 2018

Choose a reason for hiding this comment

perrozzi commented Feb 22, 2018

civanch commented Feb 22, 2018

fabiocos commented Feb 23, 2018

fabiocos commented Feb 23, 2018

SanghyunKo Mar 17, 2018

Choose a reason for hiding this comment

makortel Mar 17, 2018

Choose a reason for hiding this comment

makortel Mar 17, 2018

Choose a reason for hiding this comment

SanghyunKo Mar 17, 2018

Choose a reason for hiding this comment

SanghyunKo Mar 17, 2018 • edited

Choose a reason for hiding this comment

makortel Mar 20, 2018

Choose a reason for hiding this comment

makortel commented Feb 1, 2018 •

edited

perrozzi Feb 22, 2018 •

edited

SanghyunKo Mar 17, 2018 •

edited