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.

Sign up for GitHub

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

Jump to bottom

Move sortByStringFunction to StringObjectFunction.h. #18266

Merged
merged 1 commit into from Apr 9, 2017
Merged

Move sortByStringFunction to StringObjectFunction.h. #18266

merged 1 commit into from Apr 9, 2017

Conversation

Teemperor
Copy link
Contributor

PhysicsTools/UtilAlgos/interface/CachingVariable.h is sometimes only
included because of the sortByStringFunction template. This include
however is causing a layering violation because for example
CommonTools/UtilAlgos and PhysicsTools/UtilAlgos now include each
other.

This patch moves the sortByStringFunction template into the
StringObjectFunction.h and removes the now unnecessary includes
to CachingVariable.h

@cmsbuild cmsbuild added this to the Next CMSSW_9_1_X milestone Apr 7, 2017
@cmsbuild
Copy link
Contributor

cmsbuild commented Apr 7, 2017

A new Pull Request was created by @Teemperor (Raphael Isemann) for master.

It involves the following packages:

CommonTools/UtilAlgos
CommonTools/Utils
PhysicsTools/UtilAlgos

@perrotta, @cmsbuild, @slava77, @monttj, @davidlange6 can you please review it and eventually sign? Thanks.
@makortel this is something you requested to watch as well.
@Muzaffar, @davidlange6, @smuzaffar you are the release manager for this.

cms-bot commands are listed here #13028

@Teemperor
Copy link
Contributor Author

This is a new attempt for #18207. Part of solving issue #15248

@perrotta
Copy link
Contributor

perrotta commented Apr 8, 2017

please test

@cmsbuild
Copy link
Contributor

cmsbuild commented Apr 8, 2017 •
edited

The tests are being triggered in jenkins.
https://cmssdt.cern.ch/jenkins/job/ib-any-integration/19023/console Started: 2017/04/08 09:59

@cmsbuild
Copy link
Contributor

cmsbuild commented Apr 8, 2017

+1
Tested at: fd8cc29
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-18266/19023/summary.html

@cmsbuild
Copy link
Contributor

cmsbuild commented Apr 8, 2017

Comparison job queued.

@cmsbuild
Copy link
Contributor

cmsbuild commented Apr 8, 2017

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

Comparison Summary:

  • You potentially added 54 lines to the logs
  • Reco comparison results: 3242 differences found in the comparisons
  • DQMHistoTests: Total files compared: 23
  • DQMHistoTests: Total histograms compared: 1917277
  • DQMHistoTests: Total failures: 30506
  • DQMHistoTests: Total nulls: 0
  • DQMHistoTests: Total successes: 1886598
  • DQMHistoTests: Total skipped: 173
  • DQMHistoTests: Total Missing objects: 0
  • Checked 94 log files, 14 edm output root files, 23 DQM output files

@perrotta
Copy link
Contributor

perrotta commented Apr 8, 2017

At this point, shouldn't the include of CachingVariable.h be removed also from
PhysicsTools/UtilAlgos/interface/StringBasedNTupler.h ? Could you please check and, if so, clean up further and remove the include also from there?

@Teemperor
Move sortByStringFunction to StringObjectFunction.h.
62e382d 
PhysicsTools/UtilAlgos/interface/CachingVariable.h is sometimes only
included because of the sortByStringFunction template. This include
however is causing a layering violation because for example
CommonTools/UtilAlgos and PhysicsTools/UtilAlgos now include each
other.

This patch moves the sortByStringFunction template into the
StringObjectFunction.h and removes the now unnecessary includes
to CachingVariable.h and in StringBasedNTupler.h.
@Teemperor Teemperor force-pushed the ExtractSortByStringFunction branch from fd8cc29 to 62e382d Compare April 8, 2017 18:04
@cmsbuild
Copy link
Contributor

cmsbuild commented Apr 8, 2017

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

@Teemperor
Copy link
Contributor Author

Seems like this header has indeed also just included this for the sortByStringFunction.

@perrotta
Copy link
Contributor

perrotta commented Apr 8, 2017

please test

@cmsbuild
Copy link
Contributor

cmsbuild commented Apr 8, 2017 •
edited

The tests are being triggered in jenkins.
https://cmssdt.cern.ch/jenkins/job/ib-any-integration/19031/console Started: 2017/04/08 23:24

@cmsbuild
Copy link
Contributor

cmsbuild commented Apr 8, 2017

+1
Tested at: 62e382d
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-18266/19031/summary.html

@cmsbuild
Copy link
Contributor

cmsbuild commented Apr 8, 2017

Comparison job queued.

@cmsbuild
Copy link
Contributor

cmsbuild commented Apr 9, 2017

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

Comparison Summary:

  • No significant changes to the logs found
  • Reco comparison results: 3254 differences found in the comparisons
  • DQMHistoTests: Total files compared: 23
  • DQMHistoTests: Total histograms compared: 1917345
  • DQMHistoTests: Total failures: 30931
  • DQMHistoTests: Total nulls: 0
  • DQMHistoTests: Total successes: 1886241
  • DQMHistoTests: Total skipped: 173
  • DQMHistoTests: Total Missing objects: 0
  • Checked 94 log files, 14 edm output root files, 23 DQM output files

@perrotta
Copy link
Contributor

perrotta commented Apr 9, 2017

+1
sortByStringFunction just moved from CachingVariable.h to StringObjectFunction.h to avoid cyclic dependencies

@davidlange6 davidlange6 merged commit 9af95fe into cms-sw:master Apr 9, 2017
@Teemperor Teemperor deleted the ExtractSortByStringFunction branch April 9, 2017 13:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
analysis-pending comparison-available orp-pending pending-signatures reconstruction-approved tests-approved
Projects
None yet
Milestone
CMSSW_9_1_X
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@Teemperor @cmsbuild @perrotta @davidlange6