Manage PluginFactory plugins with unique_ptr in PhysicsTools #25914
Conversation
…ysicsTools This works for PluginFactory returning either raw pointer or unique_ptr.
|
The code-checks are being triggered in jenkins. |
|
+code-checks Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-25914/8394
|
|
A new Pull Request was created by @makortel (Matti Kortelainen) for master. It involves the following packages: PhysicsTools/IsolationAlgos @cmsbuild, @perrotta, @santocch, @slava77 can you please review it and eventually sign? Thanks. cms-bot commands are listed here |
|
@cmsbuild, please test |
|
The tests are being triggered in jenkins. |
| delete TrainProcessor::PluginFactory::get()->create( | ||
| std::string("TrainProcessor/") + name); | ||
| std::unique_ptr<PhysicsTools::TrainProcessor::Dummy> tmp{TrainProcessor::PluginFactory::get()->create( | ||
| std::string("TrainProcessor/") + name)}; | ||
| result = ProcessRegistry::create(name, id, trainer); | ||
| } catch(const cms::Exception &e) { |
There was a problem hiding this comment.
In principle the try-catch here is unnecessary, and the behavior could be preserved by using PluginFactory::tryToCreate(). On the other hand, the comment below says
// caller will have to deal with the null pointer
// in principle this will just give a slightly more
// descriptive error message (and will rethrow anyhow)
but there is no rethrow. So I'm not sure if this code should throw (keep the create() but remove try-catch as unnecessary) or not (change to tryToCreate()).
| delete VPPluginFactory::get()->create( | ||
| std::string("VarProcessor/") + name); | ||
| std::unique_ptr<PhysicsTools::VarProcessor::Dummy> tmp {VPPluginFactory::get()->create( | ||
| std::string("VarProcessor/") + name)}; | ||
| result = ProcessRegistry::create(name, calib, parent); | ||
| } catch(const cms::Exception &e) { |
There was a problem hiding this comment.
Here is a similar pattern as in https://github.com/cms-sw/cmssw/pull/25914/files#r256024015, except
- the code rethrows (so should use
create() - the code issues a LogError before rethrowing, so needs the try-catch (although I'm not so sure about the usefulness of the LogError if the exception propagates to the framework)
|
-1 Tested at: 8d1d694 You can see the results of the tests here: I found follow errors while testing this PR Failed tests: UnitTests
I found errors in the following unit tests: ---> test runtestRecoEgammaPhotonIdentification had ERRORS |
|
Comparison job queued. |
|
Comparison is ready Comparison Summary:
|
|
I wasn't able to reproduce any of the unit test failures in CMSSW_10_5_X_2019-02-05-1100 at LPC. |
makortel
changed the title
|
please test |
cmsbuild
added
comparison-pending
tests-pending
and removed
comparison-available
tests-rejected
labels
|
The tests are being triggered in jenkins. |
|
-1 Tested at: 8d1d694 You can see the results of the tests here: I found follow errors while testing this PR Failed tests: UnitTests
I found errors in the following unit tests: ---> test runtestUtilAlgos had ERRORS |
|
Comparison job queued. |
|
Comparison is ready Comparison Summary:
|
|
I ran all the failing unit tests in CMSSW_10_5_ASAN_X_2019-02-15-2300, ASAN did not report anything. |
|
+1
|
|
+1 to verify whether the unit test issue is related to the PR itself or to the observed problems in the test system |
|
merge |
In addition use
unique_ptrorshared_ptrconstructor where needed to prepare for PluginFactory returnningstd::unique_ptr. Also removed useless try-catches fromCachingVariable.Tested in CMSSW_10_5_X_2019-02-05-1100 , no changes expected.