Avoid calling the wrong constructor for EDGetTokenT #31701

Dr15Jones · 2020-10-07T16:36:51Z

PR description:

Fixed a case found by a user where the ADAPTER based constructor for EDGetTokenT was incorrectly chosen.
Converted the EDGetToken unit test to use catch2

PR validation:

Code compiles and the unit test passes.

cmsbuild · 2020-10-07T16:37:11Z

The code-checks are being triggered in jenkins.

cmsbuild · 2020-10-07T16:43:24Z

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-31701/18863

This PR adds an extra 16KB to repository

cmsbuild · 2020-10-07T16:43:49Z

A new Pull Request was created by @Dr15Jones (Chris Jones) for master.

It involves the following packages:

FWCore/Utilities

@makortel, @smuzaffar, @cmsbuild, @Dr15Jones can you please review it and eventually sign? Thanks.
@makortel, @felicepantaleo, @wddgit this is something you requested to watch as well.
@silviodonato, @dpiparo, @qliphy you are the release manager for this.

cms-bot commands are listed here

Dr15Jones · 2020-10-07T16:50:16Z

please test

cmsbuild · 2020-10-07T16:50:39Z

The tests are being triggered in jenkins.

CMSSW_11_2_X_2020-10-07-1100/slc7_amd64_gcc820: https://cmssdt.cern.ch/jenkins/job/ib-run-pr-tests/9781/console Started: 2020/10/07 18:51

makortel · 2020-10-07T18:07:18Z

FWCore/Utilities/interface/EDGetToken.h

@@ -88,7 +88,8 @@ namespace edm {

    //Needed to avoid EDGetTokenT(ADAPTER&&) from being called instead
    // when we can use C++20 concepts we can avoid the problem using a constraint
-    constexpr EDGetTokenT(EDGetTokenT& iOther) noexcept : m_value{iOther.m_value} {}
+    constexpr EDGetTokenT(EDGetTokenT<T>& iOther) noexcept : m_value{iOther.m_value} {}
+    constexpr EDGetTokenT(const EDGetTokenT<T>&& iOther) noexcept : m_value{iOther.m_value} {}


Would ESGetToken benefit from a similar fix?

Not needed since ESGetToken doesn't support the ADAPTER ability (since it doesn't know the data product type).

cmsbuild · 2020-10-07T18:45:31Z

-1

Tested at: 4a9eced

CMSSW: CMSSW_11_2_X_2020-10-07-1100
SCRAM_ARCH: slc7_amd64_gcc820

The following merge commits were also included on top of IB + this PR after doing git cms-merge-topic:

[DD4hep] Filtered View is not a Shape #31691 @ianna: [DD4hep] Filtered View is not a Shape

You can see more details here:
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-2058d3/9781/git-log-recent-commits
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-2058d3/9781/git-merge-result

You can see the results of the tests here:
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-2058d3/9781/summary.html

I found follow errors while testing this PR

Failed tests: ClangBuild

Clang:

I found compilation warning while trying to compile with clang. Command used:

USER_CUDA_FLAGS='--expt-relaxed-constexpr' USER_CXXFLAGS='-Wno-register -fsyntax-only' scram build -k -j 32 COMPILER='llvm compile'

See details on the summary page.

The following merge commits were also included on top of IB + this PR after doing git cms-merge-topic:

[DD4hep] Filtered View is not a Shape #31691 @ianna: [DD4hep] Filtered View is not a Shape

You can see more details here:
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-2058d3/9781/git-log-recent-commits
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-2058d3/9781/git-merge-result

cmsbuild · 2020-10-07T18:45:33Z

Comparison not run due to Build errors/Fireworks only changes/No short matrix requested (RelVals and Igprof tests were also skipped)

Added an additional special constructor to avoid accidentally calling the constructor taking an adapter.

cmsbuild · 2020-10-08T14:25:55Z

The code-checks are being triggered in jenkins.

cmsbuild · 2020-10-08T14:32:11Z

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-31701/18894

This PR adds an extra 16KB to repository

cmsbuild · 2020-10-08T14:32:37Z

Pull request #31701 was updated. @makortel, @smuzaffar, @cmsbuild, @Dr15Jones can you please check and sign again.

Dr15Jones · 2020-10-08T14:33:21Z

please test

cmsbuild · 2020-10-08T14:33:39Z

The tests are being triggered in jenkins.

CMSSW_11_2_X_2020-10-08-1100/slc7_amd64_gcc820: https://cmssdt.cern.ch/jenkins/job/ib-run-pr-tests/9810/console Started: 2020/10/08 16:34

cmsbuild · 2020-10-08T18:15:05Z

-1

Tested at: 6e41cd2

CMSSW: CMSSW_11_2_X_2020-10-08-1100
SCRAM_ARCH: slc7_amd64_gcc820
You can see the results of the tests here:
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-2058d3/9810/summary.html

I found follow errors while testing this PR

Failed tests: UnitTests

Unit Tests:

I found errors in the following unit tests:

---> test materialBudgetHGCalPlots had ERRORS

cmsbuild · 2020-10-08T18:15:07Z

Comparison job queued.

cmsbuild · 2020-10-08T19:41:45Z

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

Comparison Summary:

No significant changes to the logs found
Reco comparison results: 0 differences found in the comparisons
DQMHistoTests: Total files compared: 35
DQMHistoTests: Total histograms compared: 2542225
DQMHistoTests: Total failures: 1
DQMHistoTests: Total nulls: 0
DQMHistoTests: Total successes: 2542202
DQMHistoTests: Total skipped: 22
DQMHistoTests: Total Missing objects: 0
DQMHistoSizes: Histogram memory added: 0.0 KiB( 34 files compared)
Checked 149 log files, 22 edm output root files, 35 DQM output files

makortel · 2020-10-08T19:44:35Z

+1

cmsbuild · 2020-10-08T19:44:59Z

This pull request is fully signed and it will be integrated in one of the next master IBs (but tests are reportedly failing). This pull request will now be reviewed by the release team before it's merged. @silviodonato, @dpiparo, @qliphy (and backports should be raised in the release meeting by the corresponding L2)

qliphy · 2020-10-09T07:36:29Z

+1

silviodonato · 2020-10-09T08:38:43Z

merge

Changed EDGetToken test to use catch2

f591a41

cmsbuild added this to the CMSSW_11_2_X milestone Oct 7, 2020

cmsbuild added code-checks-pending comparison-pending core-pending orp-pending pending-signatures tests-pending labels Oct 7, 2020

cmsbuild added code-checks-approved and removed code-checks-pending labels Oct 7, 2020

cmsbuild added tests-started and removed tests-pending labels Oct 7, 2020

makortel reviewed Oct 7, 2020

View reviewed changes

cmsbuild added comparison-notrun tests-rejected and removed comparison-pending tests-started labels Oct 7, 2020

Fixed a corner case when constructing a EDGetTokenT

6ea5e3a

Added an additional special constructor to avoid accidentally calling the constructor taking an adapter.

Dr15Jones force-pushed the fixEDGetTokenT branch from 4a9eced to 6b98d30 Compare October 8, 2020 14:17

cmsbuild added code-checks-pending and removed code-checks-approved comparison-notrun tests-rejected labels Oct 8, 2020

cmsbuild added code-checks-pending and removed code-checks-rejected labels Oct 8, 2020

cmsbuild added code-checks-approved and removed code-checks-pending labels Oct 8, 2020

cmsbuild added tests-started and removed tests-pending labels Oct 8, 2020

cmsbuild added tests-rejected and removed tests-started labels Oct 8, 2020

cmsbuild added comparison-available and removed comparison-pending labels Oct 8, 2020

cmsbuild added core-approved fully-signed and removed core-pending pending-signatures labels Oct 8, 2020

cmsbuild added orp-approved and removed orp-pending labels Oct 9, 2020

cmsbuild merged commit 011507b into cms-sw:master Oct 9, 2020

Dr15Jones deleted the fixEDGetTokenT branch October 14, 2020 19:43

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Avoid calling the wrong constructor for EDGetTokenT #31701

Avoid calling the wrong constructor for EDGetTokenT #31701

Dr15Jones commented Oct 7, 2020

cmsbuild commented Oct 7, 2020

cmsbuild commented Oct 7, 2020

cmsbuild commented Oct 7, 2020

Dr15Jones commented Oct 7, 2020

cmsbuild commented Oct 7, 2020 •

edited

makortel Oct 7, 2020 •

edited

Dr15Jones Oct 7, 2020

cmsbuild commented Oct 7, 2020

cmsbuild commented Oct 7, 2020

cmsbuild commented Oct 8, 2020

cmsbuild commented Oct 8, 2020

cmsbuild commented Oct 8, 2020

Dr15Jones commented Oct 8, 2020

cmsbuild commented Oct 8, 2020 •

edited

cmsbuild commented Oct 8, 2020

cmsbuild commented Oct 8, 2020

cmsbuild commented Oct 8, 2020

makortel commented Oct 8, 2020

cmsbuild commented Oct 8, 2020

qliphy commented Oct 9, 2020

silviodonato commented Oct 9, 2020

Avoid calling the wrong constructor for EDGetTokenT #31701

Avoid calling the wrong constructor for EDGetTokenT #31701

Conversation

Dr15Jones commented Oct 7, 2020

PR description:

PR validation:

cmsbuild commented Oct 7, 2020

cmsbuild commented Oct 7, 2020

cmsbuild commented Oct 7, 2020

Dr15Jones commented Oct 7, 2020

cmsbuild commented Oct 7, 2020 • edited

makortel Oct 7, 2020 • edited

Choose a reason for hiding this comment

Dr15Jones Oct 7, 2020

Choose a reason for hiding this comment

cmsbuild commented Oct 7, 2020

cmsbuild commented Oct 7, 2020

cmsbuild commented Oct 8, 2020

cmsbuild commented Oct 8, 2020

cmsbuild commented Oct 8, 2020

Dr15Jones commented Oct 8, 2020

cmsbuild commented Oct 8, 2020 • edited

cmsbuild commented Oct 8, 2020

cmsbuild commented Oct 8, 2020

cmsbuild commented Oct 8, 2020

makortel commented Oct 8, 2020

cmsbuild commented Oct 8, 2020

qliphy commented Oct 9, 2020

silviodonato commented Oct 9, 2020

cmsbuild commented Oct 7, 2020 •

edited

makortel Oct 7, 2020 •

edited

cmsbuild commented Oct 8, 2020 •

edited