search

AIDASoft / podio

PODIO
GNU General Public License v3.0
24 stars 60 forks source link

Check if the data passed to the frame model is not a nullptr #578

Closed jmcarcell closed 6 months ago

jmcarcell commented 6 months ago

BEGINRELEASENOTES

ENDRELEASENOTES

It looks like a good idea to check if the pointer is null or not regardless. This makes opening corrupted or wrong files throw

terminate called after throwing an instance of 'std::invalid_argument'
  what():  Building a Frame failed; if you are reading from a file it may be corrupted

instead of crashing.

tmadlener commented 6 months ago

In principle you could also be reading past the available number of entries, as I originally envisioned the user to check whether the uniqe_ptr<FramaData> they retrieve from the reader is a valid pointer. However, I see it's not documented, so we should also add something to the docstring of the constructor here:

https://github.com/AIDASoft/podio/blob/b94cc639b884356b8eb43b017586d86ed513558a/include/podio/Frame.h#L149-L155

I would then make the exception message mainly about receiving a nullptr for the FrameData.

jmcarcell commented 6 months ago

Changed. I prefer a more verbose error message because it's a public one and easy to get either by reading more entries than there are or by unhealthy files (I don't remember exactly how but I was able to create files that reach until there but then the data is nullptr) and with the exception you don't get any information where it happened while when it's crashing with debug symbols you get some information, so it should try to not be worse than the crash.

tmadlener commented 6 months ago

Not entirely sure why the dev4 workflows are failing now. The issue looks like https://github.com/root-project/root/issues/15117 but this should have been fixed. Merging this for now.