Better exception message if module label in ESInputTag doesn't match #45947
Conversation
|
cms-bot internal usage |
|
+code-checks Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-45947/41707 |
|
A new Pull Request was created by @wddgit for master. It involves the following packages:
@Dr15Jones, @cmsbuild, @jfernan2, @makortel, @mandrenguyen, @smuzaffar can you please review it and eventually sign? Thanks. cms-bot commands are listed here |
|
please test |
| for (auto const& item : lookup_) { | ||
| if (item.productLabel_ == iProductLabel) { | ||
| if (iProductLabel.empty() or iProductLabel == item.productLabel_) { | ||
| if (iModuleLabel.empty() or iModuleLabel == item.moduleLabel_) { |
There was a problem hiding this comment.
This was the bug fix. iProductLabel -> iModuleLabel.
|
-1 Failed Tests: UnitTests Unit TestsI found 1 errors in the following unit tests: ---> test testSiStripPayloadInspector had ERRORS Comparison SummarySummary:
|
|
The unit test fails in the IB #45952 |
| constexpr ESResolverIndex(ESResolverIndex&&) noexcept = default; | ||
|
|
||
| constexpr ESResolverIndex& operator=(ESResolverIndex const&) noexcept = default; | ||
| constexpr ESResolverIndex& operator=(ESResolverIndex&&) noexcept = default; |
There was a problem hiding this comment.
@Dr15Jones Do you know if the compiler-generated copy/move constructors/assignment operators are constexpr ... noexcept by default?
There was a problem hiding this comment.
I'm thinking about this. I could just leave these 4 lines of code the way they were. It is kind of off the topic of the rest of the PR anyway. I just saw a rule of 5 violation (no destructor) and thought these were unnecessary. I'm not sure if the constexpr and noexcept are useful or not... Now that I'm looking the other two classes follow the same pattern.
There was a problem hiding this comment.
Not the simplest reading and I don't have a copy of the C++20 standard. After about an hour of research on google, I conclude the implicit copy/move constructor/assignment will be constexpr and noexcept if the content of the class allows that. For example, if the class had a data member that did not have a constexpr and noexcept copy constructor, then its implicit copy constructor would not be constexpr and noexcept. And there are lots of complex requirements for constexpr and noexcept to be possible.
In the case of ESResolverIndex, the implicit ones will be constexpr and noexcept.
The only advantage to declaring it explicitly is that if you really wanted that but made a mistake implementing your class (or later when you modified it) such that your class copy constructor could not be noexcept or constexpr, then the compiler would give an error on compilation.
@Dr15Jones I'd be happy to hear your opinion on that.
There was a problem hiding this comment.
Actually, I don't think the compiler will give any error if you declare noexcept with a function that can throw and it only might give an error or warning for a constexpr function calling a non-constexpr function but the standard does not require a diagnostic... So declaring it explicitly doesn't help much.
There was a problem hiding this comment.
Matti, Chris, and I discussed this verbally. Summary: Implicitly declared functions would be constexpr and noexcept if used because the content of this class allows that, but it is better to explicitly declare this because the compiler will give useful diagnostic warnings in some cases. Soon, I will push a commit that restores those 4 lines and restart the tests.
| }); | ||
| iData = nullptr; | ||
| return; | ||
| } |
There was a problem hiding this comment.
Is this check (and the one below) now effectively done elsewhere?
There was a problem hiding this comment.
Both of these checks for error conditions duplicate error checks made elsewhere. It is impossible for these error checks to ever fail. This is true both before and after this PR.
We already check here:
https://cmssdt.cern.ch/lxr/source/FWCore/Framework/interface/EventSetupRecord.h#0166
that iResolverIndex does not have an invalid value and that other error check is always run first.
We already check here that pValue is not null:
https://cmssdt.cern.ch/lxr/source/FWCore/Framework/src/ESProductResolver.cc#0082
And that other error check is made first so the one I deleted couldn't ever fail.
It seemed better to me to get rid of that duplication, although I suppose one might argue it's good to check inside that function because it would cause problems if something outside the function changed and either of those conditions started failing... I can put them back in if you would prefer that, although I would have to think a bit about should be in the exception message in those cases.
wddgit
force-pushed
the
eventSetupModuleLabelMismatch
branch
from
b099663 to
35ea795
Compare
cmsbuild
added
tests-pending
code-checks-pending
and removed
tests-rejected
code-checks-approved
labels
|
+code-checks Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-45947/41730 |
|
Pull request #45947 was updated. @Dr15Jones, @cmsbuild, @jfernan2, @makortel, @mandrenguyen, @smuzaffar can you please check and sign again. |
|
please test All the comments received so far should be addressed. Let me know if there are more. Note that the RECO changes are trivial, 2 lines are deleted that include an unused header file. |
|
+1 Size: This PR adds an extra 24KB to repository Comparison SummarySummary:
|
|
+1 |
|
+core |
cmsbuild
added
core-approved
fully-signed
and removed
core-pending
pending-signatures
labels
|
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. @rappoccio, @sextonkennedy, @mandrenguyen, @antoniovilela (and backports should be raised in the release meeting by the corresponding L2) |
|
+1 |
PR description:
EventSetup data is uniquely identified by data type, record type, and product label. In the ESInputTag, one can additionally add the requirement that the module producing the data product also be associated with a specific module label. Before this PR, if the EventSetup fails to get a data product the system prints the same message in the case where the data type, record type, and product label cannot be found and in the case where a matching product is found but the module label does not match. This PR modifies the exception message to explicitly identify those cases where the only problem is that the module label does not match. This makes debugging problems easier.
This also includes a bug fix for the case of a "may consumes" EventSetup request that also requires a specific module label. See the changes in ESTagGetter.cc. There is nothing in CMSSW using that feature currently so this bug shouldn't be causing any problems (unless there is code using it that isn't in the repository...).
Note that one design decision was made implementing this. It would be nice to print out the module label requested in the ESInputTag and also the module label associated with the ESProducer in the case where there is a module label mismatch. That would make debugging even easier. But that information is not currently available at the point in code execution where the exception is constructed. It would require additional memory to make it available. Upon discussion between myself and Matti, we decided that it was better to save memory than to print out this info. Mainly this was based on how little this feature is used and how rare such exceptions are. It is not hard to figure those module labels given the information that is available.
PR validation:
Extended the unit test eventsetup_t.cppunit.cc to cover the cases related to the bug in ESTagGetter.cc. Also manually tested that all the exceptions were printing out nicely and formatted as expected in cases where the exceptions get thrown.