RawBank::end() and RawBank::range() are broken

The recent MR !2994 (merged) exposed a bug (see here) in RawBank::end<T>() and RawBank::range<T>() where sizeof(T) is more than 1. The default for RawBank::range<>() happens to be unsigned int which means that the last 1-3 bytes are potentially lost.

Perhaps T should be hardcoded to char, as generalizing to other types in this place makes little sense.

added Persistency bug labels

mentioned in merge request !2994 (merged)

changed the description

do I understand correctly that the problem is that there is a default, and it would be better if there was not, eg:

diff --git a/Event/DAQEvent/Event/RawBank.h b/Event/DAQEvent/Event/RawBank.h
index abd4b48856..bbbc88313f 100644
--- a/Event/DAQEvent/Event/RawBank.h
+++ b/Event/DAQEvent/Event/RawBank.h
@@ -248,7 +248,7 @@ namespace LHCb {
     }
 
     /// return a range of 'const T'
-    template <typename T = unsigned int>
+    template <typename T>
     LHCb::span<const T> range() const {
       return {begin<T>(), end<T>()};
     }

and then fix any fall-out?

The problem is really for any sizeof(T) > 1 because

    template <typename T>
    T* end() {
      return begin<T>() + size() / sizeof( T );
    }

lies about the size by truncating it.

well, if you ask for a T with sizeof(T) larger than one, truncating is the only thing that can be done in case size() is not a multiple of sizeof(T), as otherwise it would be an out-of-bounds access to uninitialised memory. So in those cases that happens, one could say it is a mistake by the caller (i.e. the question should not have been asked)... hence my remark that the solution is presumably to make it (more) explicit to the caller to make sure they ask the right question...

why not remove the templating and always have unsigned char?

because most subdetector payloads are 32-bit (or 16-bit) unsigned integer (or a mix of them), and this would move the problem from one central place to many decoders... basically, most places select a 'subspan' of a bank for a given type, and then loop over the values, so there should be a uniform way to do that without everyone re-inventing the wheel...

fair enough!

mentioned in merge request !3021 (merged)

closed with merge request !3021 (merged)

mentioned in commit 2d47f6e4

mentioned in merge request !3036 (merged)

mentioned in commit 6bfd5f9e