-
Notifications
You must be signed in to change notification settings - Fork 4.3k
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
Remove boost lexical_cast dependency in FWCore #34976
Remove boost lexical_cast dependency in FWCore #34976
Conversation
+code-checks Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-34976/24818
|
A new Pull Request was created by @Purva-Chaudhari for master. It involves the following packages:
@makortel, @smuzaffar, @cmsbuild, @Dr15Jones can you please review it and eventually sign? Thanks. cms-bot commands are listed here |
please test |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks!
FWCore/ParameterSet/src/types.cc
Outdated
// std::cerr << "to:" << to << std::endl; | ||
} catch (boost::bad_lexical_cast&) { | ||
} catch (const std::invalid_argument&) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it would be useful to catch also std::out_of_range
. The both appear to inherit from std::logic_error
, so I'd suggest to catch either that or the std::exception
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Replacing with a catch for std::exception
in both the files.
@@ -210,7 +209,7 @@ namespace edm { | |||
pinfo.num_threads >> pinfo.itrealvalue >> pinfo.starttime >> pinfo.vsize >> pinfo.rss >> pinfo.rlim >> | |||
pinfo.startcode >> pinfo.endcode >> pinfo.startstack >> pinfo.kstkesp >> pinfo.kstkeip >> pinfo.signal >> | |||
pinfo.blocked >> pinfo.sigignore >> pinfo.sigcatch >> pinfo.wchan; | |||
} catch (boost::bad_lexical_cast& iE) { | |||
} catch (const std::invalid_argument& iE) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
std::out_of_range
should be caught also here.
+1 Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-df0944/17967/summary.html CMS StaticAnalyzer warnings: There are 1 inherits from legacy modules warnings. See https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-df0944/17967/llvm-analysis/legacy-mod-sa.txt for details. Comparison SummarySummary:
|
+code-checks Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-34976/24831
|
Pull request #34976 was updated. @makortel, @smuzaffar, @cmsbuild, @Dr15Jones can you please check and sign again. |
@cmsbuild, please test |
+1 Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-df0944/17977/summary.html CMS StaticAnalyzer warnings: There are 1 inherits from legacy modules warnings. See https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-df0944/17977/llvm-analysis/legacy-mod-sa.txt for details. Comparison SummarySummary:
|
+1 |
This pull request is fully signed and it will be integrated in one of the next master IBs (tests are also fine). This pull request will now be reviewed by the release team before it's merged. @perrotta, @dpiparo, @qliphy (and backports should be raised in the release meeting by the corresponding L2) |
+1 |
It looks like this broke one of the unit tests:
|
The value Appears like a known annoyance with I'd be inclined to revert back to |
@Purva-Chaudhari Would you be able to change |
@makortel I'm in the process of trying to see if I can find a better solution. |
see ##34999 |
Thanks @Dr15Jones for the improvements |
PR description:
Remove boost lexical cast dependency in FWCore with corresponding stl alternatives
PR validation:
Passed on scram -b runtests (FWCore)
if this PR is a backport please specify the original PR and why you need to backport that PR:
@davidlange6 @vgvassilev