Follow-up from "TDR code, aka code that would break master but is needed to run TDR tests"

The following discussions from !826 (merged) should be addressed:

@graven started a discussion:
```
auto out = std::make_unique<LHCb::Track>(seed);
```
(please avoid the keyword new if possible)
@graven started a discussion: (+2 comments)
From the implementation in PrForwardTool.cpp it is clear that the only requirement on velo is that it has begin() and end() methods, and if an iterator dereferenced it gives const LHCb::Track&. So in the case, it is better to avoid requiring a very specific container in the interface. Instead, I would propose:
```
#include "Kernel/STLExtensions.h"

virtual void extendTracks( LHCb::span<const LHCb::Track> velo, std::vector<LHCb::Track>& result , const PrFTHitHandler<PrHit>& hitHandler) const = 0;
```
@graven started a discussion:
In this bit of code, the only requirements on hitbuffer is that it has size(), operator[](int), begin(), end(), and that elements are of type size_t. The only requirements on clusters is that it has operator[](int) and that that returns const LHCb::VPLightCluster&. The only requirement on clusterIsUsed is that it has operator[](int) and that returns unsigned char&.

Hence this function can be done generically as:
```
#include "Kernel/STLExtensions.h"

void fill( LHCb::span<const LHCb::VPLightCluster> clusters,
           LHCb::span<unsigned char> clusterIsUsed,
           LHCb::span<const size_t> hitbuffer )
```
at which point the details of the storage are reduced to being contiguous, and one can experiment 'at will' with other ways of storing the data, without changing this bit of code ever again (at least, for that reason ;-)

@graven started a discussion: (+1 comment)

      auto track = std::make_unique<LHCb::Track>(*itr) ;
      StatusCode sc = StatusCode::SUCCESS ;
      if( m_initTrackStates )
        sc = m_stateinittool->fit(*track,true);
      if( sc.isSuccess() )
        alltracks.push_back(new UTrackData(track.release())) ;
      else {
        Warning("TrackStateInitTool fit failed", sc, 0 ).ignore() ;
      }
    }

better to explicitly release than to explicitly delete....

@rquaglia started a discussion: (+2 comments)

@sponce , after latest rebase I see a drop of 50% tracking effs. in velo tracking. please put back the older sorting by phi implementation:

  bool odd = false;
  for ( size_t moduleID = 0; moduleID < VeloInfo::Numbers::NModules ; ++moduleID ) {
    //In even modules you fall in the branching at -180, 180 degrees, you want to do that continuos
    odd = (moduleID %2 == 1);
    std::stable_sort( pool.begin() + offsets[moduleID], pool.begin() + offsets[moduleID +1],
                      [&odd]( const LHCb::VPLightCluster& a, const LHCb::VPLightCluster& b){
                        //sorting in phi for even modules
                        return //odd modules, change in y value, or even, with sswapped logic
			  odd * ( a.y()< 0.f && b.y() > 0.f)||
        	          !odd * ( a.y()>0.f  && b.y() < 0.f)||
                          //same y side even and odd modules, check y1/x1 < y2/x2
			  ( (a.y()* b.y()) > 0.f && (a.y()*b.x() < b.y()*a.x())  );
                          });

This resurrect the 50% loss tracks.

@cattanem commented on a discussion: (+13 comments)

The MiniBrunel config was adapted, in Brunel!428 (merged) - could not be in the same MR because it's a different project.

I think what was done is consistent. The problem is that, to decode v5 banks, you need a condition that is not in the simcond tag used produce v2 data. The default is for v5 data and assumes that you use a simcond tag consistent with v5.

BTW, as already mentioned in LHCb#6, this readout condition should be added also to LHCBCond, otherwise you will bomb in this way when you try to read real (as opposed to simulated) upgrade data....

Edited Aug 16, 2018 by Sebastien Ponce