XML: add std:: when building with tinyXML #1230
Conversation
| _sL = det.extension< SurfaceList >(false) ; | ||
| if (not _sL) { | ||
| _sL = det.addExtension<SurfaceList >( new SurfaceList( true ) ) ; | ||
| } |
There was a problem hiding this comment.
I still see error printouts, even with these changes. I haven't seen any runtime issues yet with ILD reco, I have to still check results.
There was a problem hiding this comment.
Which k4geo test are you running?
There was a problem hiding this comment.
I am running the ILD standard reco, where this also pops up after #1226
There was a problem hiding this comment.
It might call a different extension<>()
Can you pinpoint via gdb?
There was a problem hiding this comment.
In ILD reconstruction the issue is this extension here: https://github.com/iLCSoft/DDKalTest/blob/81e946b08c3d244c762714e707b640dae0b8152d/src/DDPlanarMeasLayer.cc#L110
That in turn is defined here:
DD4hep/DDRec/include/DDRec/DetectorData.h
Line 505 in b6f660a
I couldn't immediately find a similar hook as for the SurfaceList, but someone with a bit more knowledge about the internals probably has more success.
There was a problem hiding this comment.
- auto* ext = detElement.extension<dd4hep::rec::DoubleParameters>();
+ auto* ext = detElement.extension<dd4hep::rec::DoubleParameters>(false);and then if(ext) instead of catch?
There was a problem hiding this comment.
If I have a false in there, what happens with previous versions of DD4hep? IIUC, I still get an exception, right? So, I would only have to add an additional nullptr check to have it silent for newer versions?!
There was a problem hiding this comment.
It still works in previous versions of DD4hep with the false and nullptr check. The if (!alert) return 0; was already present before.
There was a problem hiding this comment.
Just for completeness: iLCSoft/DDKalTest#16 fixes things for ILD reco
Test Results 8 files 8 suites 3h 5m 43s ⏱️ Results for commit 7adc2ff. |
|
Thomas, |
|
I think Andres suggestion would make sense in this case. After all, the try-catch for this specific case is effectively there for a runtime check and not necessarily an error. On the other hand I think it might be used like this in a few other cases as well, so there might be more "error messages" popping up in other places that are not necessarily an error. At least for me it would be OK to leave it in, but I have no idea on how many times this will still show up in other places. |
|
Let's see. If there are more problems popping up, I think we should reconsider reverting the change in the object extensions. As said: I thought the change is fully backwards compatible, which obviously is not the case. |
BEGINRELEASENOTES
ENDRELEASENOTES