Modify default value of VPChannelID so that unused bits are not set
Change default value of VPChannelID to not set the unused bits in the bit pattern.
It means a VPChannelID that is default constructed and then set component by component has the identical internal bit pattern as one that is fully specified at construction time.
I also removed the return values when setting a component as this was always discarded anyway.
work with LHCb!4781 (merged) Run2Support!63 (merged)
close Boole#29 (closed)
Validated by
-
Core Software -
RTA -
Simulation
Merge request reports
Activity
assigned to @tlatham
- Resolved by Thomas Latham
Thanks for this @hcroft. I think that this ought to work ok, although I've proposed a possible alternative in an inline comment.
With regard to removing the returns, what is the motivation for dropping them? And have you checked that they are indeed discarded everywhere (including in Rec, Allen, etc.)? If we do go ahead with it, we should probably drop the return in the actual
set
function as well.@tlatham Yes I compiled Moore (inc Allen) and Gauss to check, the return values are never used. It is not to important to fix, but nobody used that and it makes it more obvious this is a "set the channel passed" then return a copy with it set, rather than only the one returned has the component set. You are right I missed the internal set function. I'll change that too, this will not change the exported machine code but will ensure everyone uses the functions the same way.
added 1 commit
- f3d3d31b - Set only useful bits in VPChannelID, but sensor = 255 when unintialised and...
- Resolved by Menglin Xu
I've changed the default behaviour to only set the necessary bits and test that the sensor is not 0xff in the asserts. It compiled OK and ran Boole and Moore (well the Moore version had crashes but not caused by this).
mentioned in issue Boole#29 (closed)
added VP label
requested review from @tlatham
assigned to @mexu
unassigned @tlatham
requested review from @mveghel
- Resolved by Maarten Van Veghel
@mveghel - this should fix the issue in Boole noted by @rmatev here just after !542 (merged) was merged but since this modifies the changes you made in that MR it would be great if you can take a look at it