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

TauWPThreshold uses exceptions in string parsing #39627

Open
makortel opened this issue Oct 5, 2022 · 5 comments
Open

TauWPThreshold uses exceptions in string parsing #39627

makortel opened this issue Oct 5, 2022 · 5 comments

Comments

@makortel
Copy link
Contributor

makortel commented Oct 5, 2022

The deep_tau::TauWPThreshold constructor uses exceptions string parsing

try {
size_t pos = 0;
value_ = std::stod(cut_str, &pos);
simple_value = (pos == cut_str.size());
} catch (std::invalid_argument&) {
} catch (std::out_of_range&) {
}

I encountered this exception being thrown many times (don't know how many because I gave up) when trying to catch a different exception, so the parse error does not seem to be as exceptional that exceptions would be justified (see https://twiki.cern.ch/twiki/bin/view/CMSPublic/SWGuideEdmExceptionUse#Exception_Handling,
https://isocpp.github.io/CppCoreGuidelines/CppCoreGuidelines#Re-throw
https://isocpp.github.io/CppCoreGuidelines/CppCoreGuidelines#Re-errors)

I'd suggest to parse the string in a way that does not use exceptions to signal an invalid value. E,g, std::from_chars() looks like a good candidate, but GCC (or libstdc++) implements it for floating point numbers only in version 11 (to which we'll hopefully switch to soon). An alternative could be std::strtod (from C) or std::istringstream (expensive).

@makortel
Copy link
Contributor Author

makortel commented Oct 5, 2022

assign reconstruction

@cmsbuild
Copy link
Contributor

cmsbuild commented Oct 5, 2022

New categories assigned: reconstruction

@mandrenguyen,@clacaputo you have been requested to review this Pull request/Issue and eventually sign? Thanks

@cmsbuild
Copy link
Contributor

cmsbuild commented Oct 5, 2022

A new Issue was created by @makortel Matti Kortelainen.

@Dr15Jones, @perrotta, @dpiparo, @rappoccio, @makortel, @smuzaffar can you please review it and eventually sign/assign? Thanks.

cms-bot commands are listed here

@clacaputo
Copy link
Contributor

type tau

@cms-sw/tau-pog-l2

@cmsbuild cmsbuild added the tau label Oct 10, 2022
@kandrosov
Copy link
Contributor

Since parsing occurs only at the initialisation stage, I think std::istringstream is a best option for now. Once std::from_chars is available - we can switch to it.

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

No branches or pull requests

4 participants