Follow-up from "Run3 File Summary Record"

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

• @sponce started a discussion: (+1 comment)

  Does it need to be in the middle of the includes ? If no, let's move it. If yes, we should fix this (in an independent MR of course)

  @clemenci

  Yes, it has to be before the first inclusion of GaudiKernel/ToStream.h otherwise Gaudi::Property picks up the wrong implementation.

  The proper fix requires refactoring of GaudiKernel/ToStream.h to use ADL to look up the right implementation of Gaudi::Utils::toStream.

• @sponce started a discussion: (+2 comments)

  I would be tempted to drop this in favor of supporting directly types without a reset method in Entity itself, via an has_reset and make_reset like is already done for mergeAndReset. At least it feels wrong to have to duplicate all the constructor for this single change of m_reset being empty.

  @clemenci

  Me too... but here the question is not so much about m_reset as it is about a missing toJSON or, to put it better, have toJSON as a free function instead of a method.

  Actually we should think about replacing the need for methods with the need from free functions (something along the lines of going from c.begin() to begin(c)), so that we can monitor anything that can be converted to JSON with a to_json(ent) and the default implementation should use the standard nlohmann::json specialization mechanism (as I did for LHCb::FSR::Monitorable).

  @graven

  👍 for using more free functions instead of methods (under the right circumstances, and toJSON is one of right ones)

• @sponce started a discussion: (+2 comments)

  Why not inheriting from BaseSink and benefiting from goodies of it (see below)

  @clemenci

  Because it didn't exist at the time I wrote this implementation.

• @sponce started a discussion: (+2 comments)

  #if defined( __clang__ )
  #  pragma clang diagnostic push
  #  pragma clang diagnostic ignored "-Wunused-function"
  #  endif

  For my own curiosity, why is this needed ?

  @clemenci

  because NLOHMANN_DEFINE_TYPE_NON_INTRUSIVE generates both to_json and from_json and we only use to_json, so clang complains about from_json not being used.

  @sponce

  What about replacing with :

     [[maybe_unused]] void from_json(const nlohmann::json&, type&);

• @rmatev started a discussion: (+1 comment)

  The fact that there is no extra structure (such as a map of counters per component) gives us the most flexibility, so I think I like that (despite making it harder to find all counters for a component).

  However, are you handling name clashes in some way (throwing an exception)?

  @clemenci

  Name clashes are, in principle, prohibited upstream. For it to happen you would need two instances of an algorithm with the same name (this reminds me that we may have clones of algorithms, in principle) or two counters with the same name in one algorithm (and now I realized I'm not sure this is checked).

  Another possible reason for a clash would be to have an algorithm called "A" with a private tool called "B" with a counter "C" at the same time as an algorithm called "A.B" with a counter "C" or an algorithm called "A" with the counter "B.C"... let's create an issue to add the check.

• @sponce started a discussion:

  I'm wondering whether this should be promoted to a "BaseAutoFlushSink", as it will be useful in the future in different cases (Online counters, generation of event display input, etc...).

  We could have also the start method there, calling a "flush" entry point one would have to implement

  @clemencic

  FTR, we discussed it offline and agreed it can be a useful feature in a base class. Not too clear if directly in BaseSink or in a BaseSink specialization.