[muon] fixed compilation warning in DataFormats/MuonReco (ArbitrationType to unsigned int) #23599
Conversation
|
The code-checks are being triggered in jenkins. |
|
+code-checks Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-23599/5202 |
|
A new Pull Request was created by @drkovalskyi for master. It involves the following packages: DataFormats/MuonReco @perrotta, @cmsbuild, @slava77 can you please review it and eventually sign? Thanks. cms-bot commands are listed here |
|
please test |
|
The tests are being triggered in jenkins. |
|
Comparison job queued. |
|
Comparison is ready Comparison Summary:
|
|
If I understood correctly what was proposed by @Dr15Jones in #23057, the main suggestion there was just to remove the comparison check, while maintaining the On the other hand, if such a possibility cannot be excluded by construction, then your solution of using |
|
This method was designed to use Muon::ArbitrationType and MuonSegmentMatch arbitration types at the same time. I agree it was probably not the best decision, but sorting it out all out now requires significantly more work. Changing the type the old functionality is preserved, i.e. it's a safe change. |
That's not a bug or pathological situation. It's a case of mixing basic arbitration types that are larger than 128 and standard type that everyone knows and uses. No warning is needed and to decide what's enough after the break one needs to review all the code carefully to understand how it all works. I don't remember it and I wouldn't risk breaking it just to silent one warning. |
|
+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. @davidlange6, @slava77, @smuzaffar, @fabiocos (and backports should be raised in the release meeting by the corresponding L2) |
what is the meaning of this? Some historical ideas from 10 years ago? Or, do we actually now find a new use cases for these methods that were not needed before? The baseline methods took In the proposed update the functional range of the method has changed, which means that they can now be called in places they could not be before. This should better be reflected in the PR title and description. Smth in line with "Restore capability to call muon::xyz methods with MuonSegmentMatch". |
|
Do we allow implicit conversion of unsigned int to enum? If not, then indeed the old functionality is disabled and I don't think we need to re-enable it, but the fact remains that we can only use the first 8 bits or we will break the code. So the most important thing is to make sure new enums are not used. I don't see how to do that beside adding a text warning as I did. |
|
So, I was partly wrong with my last comment.
On 6/19/18 7:13 AM, drkovalskyi wrote:
Do we allow implicit conversion of unsigned int to enum?
implicit no, but explicit yes.
One can still do
ArbitrationType a = (ArbitrationType)999999;
and call a reco::Muon method with it.
It will compile and will actually go to the "if (type > 1<<7)" branch of
the code.
Do we actually have these calls intentionally?
If so, then indeed, switching to the unsigned is a good way to go.
… If not, then
indeed the old functionality is disabled and I don't think we need to
re-enable it, but the fact remains that we can only use the first 8 bits
or we will break the code. So the most important thing is to make sure
new enums are not used. I don't see how to do that beside adding a text
warning as I did.
—
|
|
The only way to answer that is to forbid any reasonable action for types>128 and do a validation. I doubt that we want to do it now, but I would schedule it for next release. This is 10+ old code - I don't remember all the details. |
|
sorry, but I am missing a final decision here: the limited PR test does not show any change, but this is not necessarily fully excluding the case that Slava was mentioning, and we are not fully sure it does not happen. @slava77 @perrotta do I correctly understand that your suggestion is to integrate and see the validation response? |
|
Slava's case is what we always had. The change silences the warning and may potentially increase number of uses cases by making them explicitly possible, while there were always implicitly possible. |
|
On 6/20/18 7:59 AM, drkovalskyi wrote:
Slava's case is what we always had. The change silences the warning and
may potentially increase number of uses cases by making them explicitly
possible, while there were always implicitly possible.
—
some improvement to the PR title and/or description would be nice
|
fabiocos
changed the title
|
On 6/21/18 9:46 AM, drkovalskyi wrote:
It doesn't let me update the title.
There is an "Edit" button to the right of the PR title.
|
|
+1 |
Addressed the issue #23057. The warning raised the old design issue where a mix of basic arbitration types defined in MuonSegmentMatch and arbitration flags designed for end-users (Muon::ArbitrationType) were allowed to be used with the same interface. At the moment the only safe way to address the warning is to change the input type, which effectively doesn't change anything beside removing the warning. Long term we may want to address the issue more directly and forbid mixing arbitration types. This however requires a detailed validation, which is not wise to do so late in the release development cycle.