Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

move away from boost shared_ptrs #27841

Merged
merged 6 commits into from Sep 6, 2019

Conversation

davidlange6
Copy link
Contributor

PR description:

Removes boost::shared_ptr from cmssw code in favor of std::shared_ptr.

PR validation:

compiles and should run (it did during development, but last PR not explicitly checked

known unknown is the change to PhyscisTools/FWLite/src/classes_def.xml due to

Error: Class shared_ptrreco::parser::ExpressionBase has been selected but currently the support for its I/O is not yet available. Note that shared_ptrreco::parser::ExpressionBase, even if not selected, will be available for interpreted code.
Error: Class shared_ptrreco::parser::SelectorBase has been selected but currently the support for its I/O is not yet available. Note that shared_ptrreco::parser::SelectorBase, even if not selected, will be available for interpreted code.

which i've not found a better solution for. The unit tests of PhysicsTools/FWLite do pass after my change, but other unit tests may be interesting.

@cmsbuild
Copy link
Contributor

The code-checks are being triggered in jenkins.

@cmsbuild
Copy link
Contributor

-code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-27841/11578

  • This PR adds an extra 644KB to repository

Code check has found code style and quality issues which could be resolved by applying following patch(s)

@cmsbuild
Copy link
Contributor

The code-checks are being triggered in jenkins.

@cmsbuild
Copy link
Contributor

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-27841/11582

  • This PR adds an extra 684KB to repository

@fabiocos
Copy link
Contributor

fabiocos commented Sep 6, 2019

@igv4321 could you please comment about #27841 (comment) ? Is this code obsolete as @davidlange6 says and it appears from our repository (used nowhere)?

@fabiocos
Copy link
Contributor

fabiocos commented Sep 6, 2019

@rekovic the update to the L1 part looks straightforward to me, please have a look

@rekovic
Copy link
Contributor

rekovic commented Sep 6, 2019

+1

@cmsbuild
Copy link
Contributor

cmsbuild commented Sep 6, 2019

This pull request is fully signed and it will be integrated in one of the next master IBs after it passes the integration tests. 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)

@cmsbuild
Copy link
Contributor

cmsbuild commented Sep 6, 2019

-1

Tested at: 83519dd

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

I found follow errors while testing this PR

Failed tests: HeaderConsistency

@cmsbuild
Copy link
Contributor

cmsbuild commented Sep 6, 2019

Comparison job queued.

@fabiocos
Copy link
Contributor

fabiocos commented Sep 6, 2019

@davidlange6 as far as I can see the header consistency errors were already present at the beginning, but were not immediately apparent because failing the test due to them is a new feature added by @smuzaffar quite recently (to avoid missing the problem)

@fabiocos
Copy link
Contributor

fabiocos commented Sep 6, 2019

adding #include <memory> to GeneratorInterface/LHEInterface/interface/LHEProxy.h solves one problem

@cmsbuild
Copy link
Contributor

cmsbuild commented Sep 6, 2019

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

Comparison Summary:

  • No significant changes to the logs found
  • Reco comparison results: 0 differences found in the comparisons
  • DQMHistoTests: Total files compared: 34
  • DQMHistoTests: Total histograms compared: 2955796
  • DQMHistoTests: Total failures: 1
  • DQMHistoTests: Total nulls: 0
  • DQMHistoTests: Total successes: 2955454
  • DQMHistoTests: Total skipped: 341
  • DQMHistoTests: Total Missing objects: 0
  • DQMHistoSizes: Histogram memory added: 0.0 KiB( 33 files compared)
  • Checked 145 log files, 15 edm output root files, 34 DQM output files

@fabiocos
Copy link
Contributor

fabiocos commented Sep 6, 2019

adding #include <memory> to PhysicsTools/Utilities/interface/FunctClone.h solves part of the problem with this header

@fabiocos
Copy link
Contributor

fabiocos commented Sep 6, 2019

#include <memory> is needed also in PhysicsTools/Utilities/interface/RootFunctionAdapter.h

@smuzaffar
Copy link
Contributor

@fabiocos , @davidlange6 , as this Pr has been signed by all L2s so instead of fixing headers here I would suggest to create a separate PR to fix the header errors and restart this PR with that one.

@fabiocos
Copy link
Contributor

fabiocos commented Sep 6, 2019

@smuzaffar yes, this is what I was thinking as well to avoid restarting everything from scratch (although I could retain previous signatures as valid, but it is not nice as they are so manu). The fixes mentioned above are trivial. I am trying to figure the fix for the residual problem.

@fabiocos
Copy link
Contributor

fabiocos commented Sep 6, 2019

ok, PhysicsTools/Utilities/interface/FunctClone.h needs #include <cassert> as well

I will make a fix PR after the integration of this one tonight

@fabiocos
Copy link
Contributor

fabiocos commented Sep 6, 2019

+1

@fabiocos
Copy link
Contributor

fabiocos commented Sep 6, 2019

merge

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet