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::from_chars instead of std::regex+std::atof #40956

Closed

Conversation

makortel
Copy link
Contributor

@makortel makortel commented Mar 3, 2023

PR description:

This PR explores an alternative to #40915 to fix #40902 with std::from_chars (avoiding exceptions, use of locale, etc).

PR validation:

Code compiles.

@cmsbuild
Copy link
Contributor

cmsbuild commented Mar 3, 2023

-code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-40956/34439

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

@cmsbuild
Copy link
Contributor

cmsbuild commented Mar 3, 2023

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-40956/34440

@cmsbuild
Copy link
Contributor

cmsbuild commented Mar 3, 2023

A new Pull Request was created by @makortel (Matti Kortelainen) for master.

It involves the following packages:

  • PhysicsTools/IsolationAlgos (reconstruction)

@cmsbuild, @mandrenguyen, @clacaputo can you please review it and eventually sign? Thanks.
@perrotta, @dpiparo, @rappoccio you are the release manager for this.

cms-bot commands are listed here

@makortel
Copy link
Contributor Author

makortel commented Mar 3, 2023

@cmsbuild, please test for CMSSW_13_1_ASAN_X

@cmsbuild
Copy link
Contributor

cmsbuild commented Mar 3, 2023

+1

Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-2a4c73/31055/summary.html
COMMIT: 93aada5
CMSSW: CMSSW_13_1_ASAN_X_2023-03-01-2300/el8_amd64_gcc11
User test area: For local testing, you can use /cvmfs/cms-ci.cern.ch/week0/cms-sw/cmssw/40956/31055/install.sh to create a dev area with all the needed externals and cmssw changes.

@smuzaffar
Copy link
Contributor

please test

run tests for default (master) IB

++first;
}
const auto [ptr, ec] = std::from_chars(first, last, result);
return ec == std::errc();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
return ec == std::errc();
return ec == std::errc() and ptr == last;

I think the condition signifying "is a number" should be stricter in this case, because of the specifics of this plugin.

Looks like weight is a cms.string in the PSet in order to decide at runtime if the "weight" is (1) just a number, or (2) an expression/function.

With this PR as is, weight = cms.string("45 + 3") (or, "45 + 4*pt") would be translated internally to weight_ == 45. and usesFunction_ == false, which is not the intended behaviour (afaiu). Test code is here.

PS. Regarding

A potential downside is the larger amount of code.

I guess (didn't test) one way to reduce code is to just rely on weightExpr_ regardless (removing weight_ and usesFunction_), but maybe this is significantly less efficient in cases like weight = cms.string("45") (?).

@cmsbuild
Copy link
Contributor

cmsbuild commented Mar 5, 2023

-1

Failed Tests: RelVals-INPUT
Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-2a4c73/31077/summary.html
COMMIT: 93aada5
CMSSW: CMSSW_13_1_X_2023-03-05-0000/el8_amd64_gcc11
User test area: For local testing, you can use /cvmfs/cms-ci.cern.ch/week1/cms-sw/cmssw/40956/31077/install.sh to create a dev area with all the needed externals and cmssw changes.

RelVals-INPUT

  • 1002.31002.3_RunZeroBias2022B/step2_RunZeroBias2022B.log
  • 20834.7620834.76_TTbar_14TeV+2026D88_HLTwDIGI75e33/step2_TTbar_14TeV+2026D88_HLTwDIGI75e33.log
  • 21034.11421034.114_TTbar_14TeV+2026D88PU_OTInefficiency10PC/step2_TTbar_14TeV+2026D88PU_OTInefficiency10PC.log

Comparison Summary

Summary:

  • You potentially removed 14 lines from the logs
  • Reco comparison results: 6 differences found in the comparisons
  • DQMHistoTests: Total files compared: 49
  • DQMHistoTests: Total histograms compared: 3529727
  • DQMHistoTests: Total failures: 3
  • DQMHistoTests: Total nulls: 0
  • DQMHistoTests: Total successes: 3529702
  • DQMHistoTests: Total skipped: 22
  • DQMHistoTests: Total Missing objects: 0
  • DQMHistoSizes: Histogram memory added: 0.0 KiB( 48 files compared)
  • Checked 213 log files, 164 edm output root files, 49 DQM output files
  • TriggerResults: no differences found

@makortel
Copy link
Contributor Author

makortel commented Mar 7, 2023

Superseded by #40984

@makortel makortel closed this Mar 7, 2023
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.

[ASAN] global-buffer-overflow in std::regex construction
4 participants