Alignment geometry upgrade 81X #13976
Conversation
…te param (was only used for one class)
…Alignables depending on the currently loaded geometry
…package. adding trackerTopology method back
- added back the distinction between Module and ModuleUnit -> different things for strip modules - removed unused variable 'moduleName' in pixel and strip build routines
|
A new Pull Request was created by @vbotta (valeria botta) for CMSSW_8_1_X. It involves the following packages: Alignment/CommonAlignment @cmsbuild, @mmusich, @franzoni, @cerminar, @davidlange6 can you please review it and eventually sign? Thanks. cms-bot commands are list here #13028 |
|
please test |
|
The tests are being triggered in jenkins. |
|
@mmusich Is there anything left here? I was on vacation right after this PR, so I missed to check it. |
|
@ghellwig, no it is fine. |
|
+1 |
cmsbuild
added
alca-approved
fully-signed
and removed
alca-pending
pending-signatures
labels
|
This pull request is fully signed and it will be integrated in one of the next CMSSW_8_1_X IBs (tests are also fine). This pull request requires discussion in the ORP meeting before it's merged. @slava77, @davidlange6, @Degano, @smuzaffar |
|
thanks! |
|
|
||
| AlignableIndexer alignableIndexer; | ||
|
|
||
| std::vector<AlignmentLevel*> alignmentLevels; |
There was a problem hiding this comment.
could this become an unique_ptr? (otherwise you need to clean up the memory in this class, as it seems to get ownership of the pointers)
There was a problem hiding this comment.
@davidlange6, if I understand the code correctly, the memory is cleaned up here:
https://github.com/vbotta/cmssw/blob/AlignmentGeometryUpgrade_81X/Alignment/TrackerAlignment/src/TrackerAlignmentLevelBuilder.cc#L32-L42
However, using smart pointers should, of course, be preferred. But it should be a shared_ptr in this case, if I am not mistaken.
There was a problem hiding this comment.
I think, my comment was wrong because the objects deleted in the code referenced by me are vectors of pointers to std::vector<AlignmentLevel*>. Need to check further, how to fix that...
There was a problem hiding this comment.
@davidlange6, my initial comment was right, i.e. the objects get deleted in the destructor of 'TrackerAlignmentLevelBuilder'
Since, @vbotta is currently ill, we would get some delay until she pushed the changes to use smart pointers here. That is, in case of no exception we have no leak, so should we proceed now and update the code in a separate PR?
|
@ghellwig please yes. This one is holding other PRs. |
|
@davidlange6, if you agree can you please approve this? |
|
+1 |
|
@davidlange6, thank you. |
|
Just to make sure: the style comment applies to the whole alignment package. |
Upgraded alignment framework to cope with Phase1 geometry.