Fix "Called C++ object pointer is null" in DDCoreToDDXMLOutput::position #46075
Conversation
|
please test |
|
cms-bot internal usage |
|
+code-checks Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-46075/41865 |
|
A new Pull Request was created by @iarspider for master. It involves the following packages:
@Dr15Jones, @bsunanda, @civanch, @kpedro88, @makortel, @mdhildreth can you please review it and eventually sign? Thanks. cms-bot commands are listed here |
|
+1 Size: This PR adds an extra 20KB to repository Comparison SummarySummary:
|
| @@ -784,25 +784,27 @@ void DDCoreToDDXMLOutput::position(const TGeoVolume& parent, | |||
| xos << "<rChild name=\"" << childVolName << "\"/>" << std::endl; | |||
|
|
|||
| const auto matrix = child.GetMatrix(); | |||
| if (matrix != nullptr && !matrix->IsIdentity()) { | |||
There was a problem hiding this comment.
@iarspider, if (matrix != nullptr && !matrix->IsIdentity()) is a valid construction. If the first check 'false' the second does not checked. This is used in many places, no reason to change this place. The fix should be done in place where position is called - matrix should not be nullptr. With the current change of indentation I cannot understand where and how - better to avoid such big modification.
There was a problem hiding this comment.
@civanch I have split the condition to put https://github.com/cms-sw/cmssw/pull/46075/files#diff-de5a97aa3932a8d881fe699bde286ceabe88e31003ce97291f0ac2a7dba8d5bdR801-R806 inside if matrix check
There was a problem hiding this comment.
A rotation matrix may or may not be present in a transformation matrix. Sometimes a child is placed in a mother with no translation and no rotation. In Geant4 one needs to provide a translation vector with a default 3-vector for the translation matrix while in DD4hep one may specify the placement dropping reference to both translation and rotation
There was a problem hiding this comment.
@bsunanda , should we review this code or accept this fix?
|
@bsunanda , normally in Geant4 it is allowed to have translations without rotation. In user code it is according to concrete use case. Potentially rotation may be nullptr but translation - not. Can you , please, comment on this PR? I agree with formal static analysis but am not sure this is what we want to have. |
|
@bsunanda , we may approve this fix and make an issue to double check the logic of the code. Would it be a solution? |
|
Vlaidimir, I accept your opinion on this.
…________________________________
From: Vladimir Ivantchenko ***@***.***>
Sent: 23 September 2024 08:19
To: cms-sw/cmssw ***@***.***>
Cc: Sunanda Banerjee ***@***.***>; Mention ***@***.***>
Subject: Re: [cms-sw/cmssw] Fix "Called C++ object pointer is null" in DDCoreToDDXMLOutput::position (PR #46075)
@bsunanda<https://github.com/bsunanda> , we may approve this fix and make an issue to double check the logic of the code. Would it be a solution?
—
Reply to this email directly, view it on GitHub<#46075 (comment)>, or unsubscribe<https://github.com/notifications/unsubscribe-auth/ABGMZOX3SBCQI7CEMYWVEE3ZX56TVAVCNFSM6AAAAABOSEOX6SVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDGNRXGE2DIMZYHE>.
You are receiving this because you were mentioned.Message ID: ***@***.***>
|
|
+1 A new issue #46102 is created. |
cmsbuild
added
geometry-approved
fully-signed
and removed
geometry-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. @antoniovilela, @rappoccio, @sextonkennedy, @mandrenguyen (and backports should be raised in the release meeting by the corresponding L2) |
|
+1 |
PR description:
LLVM Analyzer reports "Called C++ object pointer is null" in
DDCoreToDDXMLOutput::position. This PR protects from this.PR validation:
Bot tests