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

use std::vector<T>::operator[] in edm::HLTGlobalStatus::operator[] #40458

Merged
merged 2 commits into from
Jan 13, 2023

Conversation

missirol
Copy link
Contributor

@missirol missirol commented Jan 9, 2023

PR description:

This PR modifies the implementation of the methods edm::HLTGlobalStatus::operator[], to use std::vector<T>::operator[] rather than std::vector<T>::at (the methods edm::HLTGlobalStatus::at remain available, and use std::vector<T>::at).

Arguably, the DataFormat should allow access to the faster std::vector<T>::operator[], leaving it to users to choose the method (at or []) most appropriate to their use case.

Reminder: edm::HLTGlobalStatus is a base class of edm::TriggerResults.

FYI: @cms-sw/hlt-l2

PR validation:

None.

If this PR is a backport, please specify the original PR and why you need to backport that PR. If this PR will be backported, please specify to which release cycle the backport is meant for:

N/A

@cmsbuild
Copy link
Contributor

cmsbuild commented Jan 9, 2023

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-40458/33616

  • This PR adds an extra 16KB to repository

@cmsbuild
Copy link
Contributor

cmsbuild commented Jan 9, 2023

A new Pull Request was created by @missirol (Marino Missiroli) for master.

It involves the following packages:

  • DataFormats/Common (core)

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

cms-bot commands are listed here

@makortel
Copy link
Contributor

makortel commented Jan 9, 2023

@cmsbuild, please test

@makortel
Copy link
Contributor

makortel commented Jan 9, 2023

In principle I'm not against. I see the implementation of operator[] using at() was part of the initial implementation in 47664bd (with commit message indicating it coming from @Martin-Grunewald)

My only concern is that this changes ~17 year old behavior in a way that can, in principle, result in undefined behavior in code that should now call the at() instead. So I wonder if all the callers should be checked (according to DXR the list is not very long), and either be convinced the [] can be called safely or the calls be changed to at()?

@cmsbuild
Copy link
Contributor

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

@makortel
Copy link
Contributor

Thanks @missirol for the extensive checks!

@makortel
Copy link
Contributor

@cmsbuild, please test

@cmsbuild
Copy link
Contributor

-1

Failed Tests: HeaderConsistency
Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-b86240/29921/summary.html
COMMIT: 453dd74
CMSSW: CMSSW_13_0_X_2023-01-11-1100/el8_amd64_gcc11
User test area: For local testing, you can use /cvmfs/cms-ci.cern.ch/week1/cms-sw/cmssw/40458/29921/install.sh to create a dev area with all the needed externals and cmssw changes.

Comparison Summary

There are some workflows for which there are errors in the baseline:
140.01 step 3
140.03 step 3
The results for the comparisons for these workflows could be incomplete
This means most likely that the IB is having errors in the relvals.The error does NOT come from this pull request

Summary:

  • No significant changes to the logs found
  • Reco comparison results: 1 differences found in the comparisons
  • DQMHistoTests: Total files compared: 49
  • DQMHistoTests: Total histograms compared: 3555538
  • DQMHistoTests: Total failures: 3
  • DQMHistoTests: Total nulls: 0
  • DQMHistoTests: Total successes: 3555513
  • DQMHistoTests: Total skipped: 22
  • DQMHistoTests: Total Missing objects: 0
  • DQMHistoSizes: Histogram memory added: 0.0 KiB( 48 files compared)
  • Checked 211 log files, 162 edm output root files, 49 DQM output files
  • TriggerResults: no differences found

@makortel
Copy link
Contributor

Header consistency check is failing because #40474 was not included in the test.

@makortel
Copy link
Contributor

release-note: This means that edm::HLTGlobalStatus::operator[]() and edm::TriggerResults::operator[]() no longer throw an exception in case the argument index is out of bounds. If the bounds-check is needed, please replace operator[]() call with at().

@makortel
Copy link
Contributor

+1

@cmsbuild
Copy link
Contributor

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. @perrotta, @dpiparo, @rappoccio (and backports should be raised in the release meeting by the corresponding L2)

@rappoccio
Copy link
Contributor

please test

@cmsbuild
Copy link
Contributor

+1

Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-b86240/29947/summary.html
COMMIT: 453dd74
CMSSW: CMSSW_13_0_X_2023-01-12-1100/el8_amd64_gcc11
User test area: For local testing, you can use /cvmfs/cms-ci.cern.ch/week1/cms-sw/cmssw/40458/29947/install.sh to create a dev area with all the needed externals and cmssw changes.

Comparison Summary

There are some workflows for which there are errors in the baseline:
11634.15 step 3
The results for the comparisons for these workflows could be incomplete
This means most likely that the IB is having errors in the relvals.The error does NOT come from this pull request

Summary:

  • No significant changes to the logs found
  • Reco comparison results: 12 differences found in the comparisons
  • DQMHistoTests: Total files compared: 49
  • DQMHistoTests: Total histograms compared: 3555538
  • DQMHistoTests: Total failures: 6
  • DQMHistoTests: Total nulls: 0
  • DQMHistoTests: Total successes: 3555510
  • DQMHistoTests: Total skipped: 22
  • DQMHistoTests: Total Missing objects: 0
  • DQMHistoSizes: Histogram memory added: 0.0 KiB( 48 files compared)
  • Checked 211 log files, 162 edm output root files, 49 DQM output files
  • TriggerResults: no differences found

@perrotta
Copy link
Contributor

+1

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.

5 participants