[10.6.X][TRACK] fix for clang warnings #26167

smuzaffar · 2019-03-13T12:29:09Z

PR description:

This fixes clang warnings for packages under Track* subsystem

PR validation:

Local clang build looks good

if this PR is a backport please specify the original PR:

Before submitting your pull requests, make sure you followed this checklist:

verify that the PR is really intended for the chosen branch
verify that changes follow CMS Naming, Coding, And Style Rules
verify that the PR passes the basic test procedure suggested in the CMSSW PR instructions

smuzaffar · 2019-03-13T12:29:16Z

please test

cmsbuild · 2019-03-13T12:29:35Z

The code-checks are being triggered in jenkins.

cmsbuild · 2019-03-13T12:35:12Z

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-26167/8761

This PR adds an extra 36KB to repository

cmsbuild · 2019-03-13T12:35:42Z

The tests are being triggered in jenkins.
https://cmssdt.cern.ch/jenkins/job/ib-any-integration/33570/console Started: 2019/03/13 14:50

cmsbuild · 2019-03-13T12:35:44Z

A new Pull Request was created by @smuzaffar (Malik Shahzad Muzaffar) for master.

It involves the following packages:

TrackPropagation/SteppingHelixPropagator
TrackingTools/TrackRefitter

@cmsbuild, @perrotta, @slava77 can you please review it and eventually sign? Thanks.
@bellan, @makortel, @felicepantaleo, @abbiendi, @GiacomoSguazzoni, @jhgoh, @VinInn, @calderona, @HuguesBrun, @rovere, @gpetruc, @battibass, @ebrondol, @trocino, @dgulhan, @folguera this is something you requested to watch as well.
@davidlange6, @slava77, @fabiocos you are the release manager for this.

cms-bot commands are listed here

cmsbuild · 2019-03-13T18:22:29Z

+1
Tested at: 670e6c2
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-26167/33570/summary.html

The following merge commits were also included on top of IB + this PR after doing git cms-merge-topic:
1313262
40d2157
636ce88
ac146f1
821cba0
2ec70ea
35678a3
defc586
c19a5d8
7bbacb1
You can see more details here:
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-26167/33570/git-log-recent-commits
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-26167/33570/git-merge-result

cmsbuild · 2019-03-13T18:22:36Z

Comparison job queued.

cmsbuild · 2019-03-13T20:20:08Z

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

Comparison Summary:

No significant changes to the logs found
Reco comparison results: 5016 differences found in the comparisons
DQMHistoTests: Total files compared: 32
DQMHistoTests: Total histograms compared: 3114829
DQMHistoTests: Total failures: 720
DQMHistoTests: Total nulls: 2
DQMHistoTests: Total successes: 3113910
DQMHistoTests: Total skipped: 197
DQMHistoTests: Total Missing objects: 0
DQMHistoSizes: Histogram memory added: 0.004 KiB( 31 files compared)
DQMHistoSizes: changed ( 136.85 ): 0.004 KiB JetMET/SUSYDQM
Checked 133 log files, 14 edm output root files, 32 DQM output files

slava77 · 2019-03-13T20:41:51Z

TrackingTools/TrackRefitter/src/TrackTransformerForCosmicMuons.cc

@@ -341,7 +341,7 @@ vector<Trajectory> TrackTransformerForCosmicMuons::transform(const reco::Track&
    return vector<Trajectory>();
  }

-  return trajectoriesSM;
+  return std::move(trajectoriesSM);


I recall that in similar other cases we switched to lvalue instead of introducing a move.

The warning is

TrackingTools/TrackRefitter/src/TrackTransformerForCosmicMuons.cc:344:10: warning: local variable 'trajectoriesSM' will be copied despite being returned by name [-Wreturn-std-move]

I suppose this is because of it being an rvalue-reference vector<Trajectory> && trajectoriesSM = ... on line 337 above.

It would likely be clearer to drop the rvalue reference, i.e. change to vector<Trajectory> trajectoriesSM = ..., and keep returning the trajectoriesSM by value (relying on NRVO).

@makortel
thanks.
that's exactly what I meant, in a telegram style.

@smuzaffar
please clarify if you are going to update this PR following the review comments.
Thank you.

@slava77 , @makortel I have updated it to use const vector<Trajectory> &trajectoriesSM. Let me know if you really want to use vector<Trajectory> trajectoriesSM instead.

By thr way, you we need line number 341? why not just return trajectoriesSM even if it is empty?

I believe it really should be vector<Trajectory> (i.e. value instead of const reference) for the NRVO to apply (or at least that's what a quick test on godbolt suggests).

By thr way, you we need line number 341? why not just return trajectoriesSM even if it is empty?

I was wondering the same and would just return trajectoriesSM (probably some historical leftover).

cmsbuild · 2019-03-19T05:57:26Z

The code-checks are being triggered in jenkins.

cmsbuild · 2019-03-19T06:03:09Z

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-26167/8829

This PR adds an extra 36KB to repository

cmsbuild · 2019-03-19T06:03:37Z

The tests are being triggered in jenkins.
https://cmssdt.cern.ch/jenkins/job/ib-any-integration/33658/console Started: 2019/03/19 08:25

cmsbuild · 2019-03-19T06:03:39Z

Pull request #26167 was updated. @cmsbuild, @perrotta, @slava77 can you please check and sign again.

cmsbuild · 2019-03-19T08:46:23Z

+1
Tested at: ce22e41
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-26167/33658/summary.html

cmsbuild · 2019-03-19T08:46:28Z

Comparison job queued.

cmsbuild · 2019-03-19T09:49:23Z

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

Comparison Summary:

No significant changes to the logs found
Reco comparison results: 0 differences found in the comparisons
DQMHistoTests: Total files compared: 32
DQMHistoTests: Total histograms compared: 3114829
DQMHistoTests: Total failures: 1
DQMHistoTests: Total nulls: 0
DQMHistoTests: Total successes: 3114631
DQMHistoTests: Total skipped: 197
DQMHistoTests: Total Missing objects: 0
DQMHistoSizes: Histogram memory added: 0.0 KiB( 31 files compared)
Checked 133 log files, 14 edm output root files, 32 DQM output files

slava77 · 2019-03-19T14:13:31Z

+1

for #26167 ce22e41

technical
jenkins tests pass

cmsbuild · 2019-03-19T14:14:00Z

This pull request is fully signed and it will be integrated in one of the next master IBs (tests are also fine). This pull request will now be reviewed by the release team before it's merged. @davidlange6, @slava77, @smuzaffar, @fabiocos (and backports should be raised in the release meeting by the corresponding L2)

fabiocos · 2019-03-19T21:26:11Z

+1

clang warnings fixed

670e6c2

cmsbuild added this to the CMSSW_10_6_X milestone Mar 13, 2019

cmsbuild added code-checks-pending comparison-pending orp-pending pending-signatures reconstruction-pending tests-pending labels Mar 13, 2019

cmsbuild added code-checks-approved and removed code-checks-pending labels Mar 13, 2019

cmsbuild added tests-started and removed tests-pending labels Mar 13, 2019

cmsbuild added tests-approved and removed tests-started labels Mar 13, 2019

cmsbuild added comparison-available and removed comparison-pending labels Mar 13, 2019

slava77 reviewed Mar 13, 2019

View reviewed changes

avoid adding std::move

f6e4cb3

cmsbuild added code-checks-pending and removed code-checks-approved comparison-available tests-approved labels Mar 18, 2019

cmsbuild added code-checks-pending comparison-pending tests-pending and removed comparison-available tests-approved labels Mar 19, 2019

cmsbuild added code-checks-approved and removed code-checks-pending labels Mar 19, 2019

cmsbuild added tests-started and removed tests-pending labels Mar 19, 2019

cmsbuild added tests-approved and removed tests-started labels Mar 19, 2019

cmsbuild added comparison-available and removed comparison-pending labels Mar 19, 2019

cmsbuild added fully-signed reconstruction-approved and removed pending-signatures reconstruction-pending labels Mar 19, 2019

cmsbuild added orp-approved and removed orp-pending labels Mar 19, 2019

cmsbuild merged commit ee428a9 into cms-sw:master Mar 19, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[10.6.X][TRACK] fix for clang warnings #26167

[10.6.X][TRACK] fix for clang warnings #26167

smuzaffar commented Mar 13, 2019

smuzaffar commented Mar 13, 2019

cmsbuild commented Mar 13, 2019

cmsbuild commented Mar 13, 2019

cmsbuild commented Mar 13, 2019 •

edited

cmsbuild commented Mar 13, 2019

cmsbuild commented Mar 13, 2019

cmsbuild commented Mar 13, 2019

cmsbuild commented Mar 13, 2019

slava77 Mar 13, 2019

makortel Mar 13, 2019

slava77 Mar 13, 2019

slava77 Mar 18, 2019

smuzaffar Mar 18, 2019

makortel Mar 18, 2019

cmsbuild commented Mar 19, 2019

cmsbuild commented Mar 19, 2019

cmsbuild commented Mar 19, 2019 •

edited

cmsbuild commented Mar 19, 2019

cmsbuild commented Mar 19, 2019

cmsbuild commented Mar 19, 2019

cmsbuild commented Mar 19, 2019

slava77 commented Mar 19, 2019

cmsbuild commented Mar 19, 2019

fabiocos commented Mar 19, 2019

[10.6.X][TRACK] fix for clang warnings #26167

[10.6.X][TRACK] fix for clang warnings #26167

Conversation

smuzaffar commented Mar 13, 2019

PR description:

PR validation:

if this PR is a backport please specify the original PR:

smuzaffar commented Mar 13, 2019

cmsbuild commented Mar 13, 2019

cmsbuild commented Mar 13, 2019

cmsbuild commented Mar 13, 2019 • edited

cmsbuild commented Mar 13, 2019

cmsbuild commented Mar 13, 2019

cmsbuild commented Mar 13, 2019

cmsbuild commented Mar 13, 2019

slava77 Mar 13, 2019

Choose a reason for hiding this comment

makortel Mar 13, 2019

Choose a reason for hiding this comment

slava77 Mar 13, 2019

Choose a reason for hiding this comment

slava77 Mar 18, 2019

Choose a reason for hiding this comment

smuzaffar Mar 18, 2019

Choose a reason for hiding this comment

makortel Mar 18, 2019

Choose a reason for hiding this comment

cmsbuild commented Mar 19, 2019

cmsbuild commented Mar 19, 2019

cmsbuild commented Mar 19, 2019 • edited

cmsbuild commented Mar 19, 2019

cmsbuild commented Mar 19, 2019

cmsbuild commented Mar 19, 2019

cmsbuild commented Mar 19, 2019

slava77 commented Mar 19, 2019

cmsbuild commented Mar 19, 2019

fabiocos commented Mar 19, 2019

cmsbuild commented Mar 13, 2019 •

edited

cmsbuild commented Mar 19, 2019 •

edited