Use invalid defaults for EvenID parameter inits

Since zero can be a valid number for various EventID related parameters, eg run# or event#, we need invalid defaults to be able to distinguish between EventIDs that are set with lumiblock info, vs run# info

Approved this merge request

Can I suggest to keep the 'special' value in the definition of a small dedicated class, i.e. something along the lines of:

  template <typename T, T empty_value>
  class compact_optional {
      T m_val;
  public:
      constexpr compact_optional() : m_val{empty_value} {}
      constexpr compact_optional(const T& v) : m_val{v} {}
      constexpr compact_optional(T&& v) : m_val{std::move(v)} {}
      constexpr explicit operator bool() const noexcept { return m_val!=empty_value; }
      const T& value() const noexcept { if (UNLIKELY(!bool(*this))) throw std::logic_error("attempt to access empty compact_optional" );
                                        return m_val; }
      constexpr const T& operator*() const { return m_val; }
      compact_optional& operator=(const T& v) { m_val = v; return *this; }
      compact_optional& operator=(T&& v) { m_val = std::move(v); return *this; } 
      compact_optional& reset() { m_val = empty_value; return *this; }
  };
   using event_number_t = compact_optional< uint64_t, std::numeric_limits<uint64_t>::max()>;
   using number_type = compact_optional< unsigned int, std::numeric_limits<unsigned_int>::max()>;

Doing it this way avoids anyone having to know the actual 'special invalid' value : it can be hidden in a using statement which would replace the typedef...

Adding a small utility class like the above will make it easier to re-use, as I'm sure this will be useful elsewhere.

(note: the above is adapted from https://akrzemi1.wordpress.com/2015/07/15/efficient-optional-values)

this is kinda clever!

the only problem with this is that we would need many more constructors for the EventIDBase, as i don't see a way to handle setting the default arguments to the invalid state.

Just specify the default argument as = { } or = event_number_t{} (i.e. instead of where there would now be = INVALID) in which case the default constructor is called, which sets it to the invalid state ;-) . So there is no need for more constructors (unless I misinterpret your question).

wouldn't it set it to the default constructor of even_number_t which is a uint_64, which is 0?

also, I just noticed that if you set the empty_value to ~std::numeric_limits<unsigned int>::max() (ie the destructor as you indicated), the empty_value is 0 not max().

No, the default c'tor explicitly sets it to the empty value:

constexpr compact_optional() : m_val{empty_value} {}

For an unsigned quantity, ~0 is the same as its max(), and thus ~~0 == 0 is true...

empirical evidence indicates the opposite on both counts.

the only way to initialize the lumi_block with an invalid value is to expose the compact_optional in the constructor. ie if we do this:

typedef unsigned int num_t;
typedef compact_optional<num_t, std::numeric_limits<num_t>::max()> co_t;

class Test {
public:
  Test (num_t i={}) : m_i(i) {};
  void prt() const {
    std::cout << "valid: " << m_i.valid() << "  empty: " << m_i.empty();
    if ( m_i.valid() ) 
      std::cout << "  val: " << m_i.value();
    std::cout << std::endl;
  }
private:
  co_t m_i = {};
};

main() {
  Test a1(1), a2(0), a3;
  a1.prt();
  a2.prt();
  a3.prt();
}

(I added an empty() method to compact_optional to print out the empty_value.) executing this will give:

valid: 1  empty: 4294967295  val: 1
valid: 1  empty: 4294967295  val: 0
valid: 1  empty: 4294967295  val: 0

notice how a3 is valid, with a value of 0, which is not what we want. if we change the constructor to:

Test(co_t i={}) : m_i(i) {}

then we get what we want

valid: 1  empty: 4294967295  val: 1
valid: 1  empty: 4294967295  val: 0
valid: 0  empty: 4294967295

however we've now exposed the compact_optional in the interface, which we don't want.

furthermore, if we change the empty_value to

typedef compact_optional<num_t, ~std::numeric_limits<num_t>::max()> co_t;

then the result is:

valid: 1  empty: 0  val: 1
valid: 0  empty: 0
valid: 0  empty: 0

ie, the value of empty_value is 0, not std::numeric_limits::max()

This:

  Test (num_t i={}) : m_i(i) {};

does not default-construct m_i. You are default-constructing a num_t, which yields a zero-valued i, and you are using this i to call the constructor of the compact_optional<num_t,...>, which then becomes valid, with the same value as i, i.e. 0..

The solution is to not provide a default value, and instead add a default constructor for Test, i.e.

  Test (num_t i) : m_i(i) {};
  Test() = default;

And then you get:

#include <utility>
#include <iostream>
#include <limits>

template <typename T, T empty_value>
class compact_optional {
    T m_val;
public:
    constexpr compact_optional() : m_val{empty_value} {}
    constexpr compact_optional(const T& v) : m_val{v} {}
    constexpr compact_optional(T&& v) : m_val{std::move(v)} {}
    constexpr explicit operator bool() const noexcept { return m_val!=empty_value; }
    const T& value() const { if (!*this) throw std::logic_error("attempt to access empty compact_optional" );
                             return m_val; }
    constexpr const T& operator*() const { return m_val; }
    compact_optional& operator=(const T& v) { m_val = v; return *this; }
    compact_optional& operator=(T&& v) { m_val = std::move(v); return *this; } 
    compact_optional& reset() { m_val = empty_value; return *this; }
};

typedef unsigned int num_t;
typedef compact_optional<num_t, std::numeric_limits<num_t>::max()> co_t;

class Test {
public:
  Test () = default;
  Test (num_t i) : m_i(i) {};
  void prt() const {
    std::cout << "valid: " << std::boolalpha << bool(m_i) ;
    if ( m_i ) std::cout << "  val: " << m_i.value();
    std::cout << std::endl;
  }
private:
  co_t m_i = {};
};

int main() {
  Test a1(1), a2(0), a3;
  a1.prt();
  a2.prt();
  a3.prt();
}

which prints -- see http://coliru.stacked-crooked.com/a/d3041788e0994fc7 -- as expected:

valid: true  val: 1
valid: true  val: 0
valid: false

Note that, if you do not want 'user code' to know about compact_optional, you have to expose the invalid value, as how would a 'user' ever know (s)he got an invalid value?

yes, that's precisely what i wrote a few messages back, and could not understand how you were claiming otherwise. so now we need multiple constructors:

EventIDBase() = default;
EventIDBase(run_number,  event_number);
EventIDBase(run_number, event_number, time_stamp, time_stamp_ns=0);
EventIDBase(run_number, event_number, time_stamp, time_stamp_ns, lumi_block, bunch_crossing_id=0);

private:
co_t m_run_number = {};
co_t m_event_number = {};
co_t m_time_stamp = {};
num_t m_time_stamp_ns {0};
co_t m_lumi_block = {};
num_t m_bunch_crossing_id {0};

i think you were writing about setting the default value in the declaration of the member variables, whereas i was referring to the default arguments in the constructor.

as for knowing about valid/invalid, the user needs to test isRunEvent(), isLumiEvent(), and isTimeStamp()

Because what I had in mind is to use the conditional_optional in the interface -- including as constructor arguments, i.e. what I had in mind was using event_number_t =compact_optional<uint 64,~0> (as I had in the initial code snippet) and not typedef uint64 event_number_t....

Now, the other option, if you really want to keep the outside interface the same, is to still define UNDEFEVT as ~0u, and use that in the definition of the optional i.e. compact_optional<event_number_t, UNDEFEVT>, and use UNDEFEVT as default constructor argument. At that point the compact_optional is only ever used internally as an implementation detail.

A third option is to actually allow for a piece-meal migration by adding an implicit conversion operator from compact_optional<T,empty_val> to T: code that wants a compact_optional<T,empty_val> as argument would require one 'by value' in which case, when invoked with a T, the constructor from T is called -- and if that T happens to be an empty_val value, it will construct an empty optional --, and code which wants a T and is invoked with a compact_optional<T,empty_val> will invoke the implicit conversion -- and as long as the conversion of empty compact_optional to a T yields an empty_val, nothing changes. So this would provide full interoperability between 'old' and 'new' code. And the implicit conversion could be flagged as deprecated to encourage people to move to compact_optional natively.

Added 1 commit:

• 1a071f04 - used compact_optional<T> to handle invalid default initialziers

I'd go one step further, and write the 'interface' in terms of compact_optional. Because what happens if the event number is 'invalid'? This would return the 'special invalid' values, at which point the caller would have to know this. So I'd just return 'm_event_number' as is, including its type, and allow the caller to use it as such.

you can't do this because it can't implicitly convert a compact_optional<number_type> to a number_type.

also, I don't want to change all the return types to compact_optional<T>, as there are Atlas classes which inherit from EventIDBase, and it would require a lot of downstream changes.

Indeed, my suggestion was to change eg. number_type to be an compact_optional itself, and then adapt the downstream code -- it would probably benefit from the change, but it is correct that this cannot be done an an adiabatic way, it is disruptive.

mentioned in commit lhcb/Rec@bb8f40ed

For my understanding, is it worth using compact_optional only partially?

I mean, if anyway the special value has to be exposed in the interface, do we gain enough just using it internally?

I guess so, but I find it difficult to judge from the diff.

Milestone changed to %v28r0

Reassigned to @clemenci

It is not clear to me why std::optional wouldn't just do the job. There is no need to reinvent the wheel if there isn't a striking reason against it. Gcc49 already has it in the experimental namespace so no problem in picking it up. This long thread already tells me that doing something consistent is non-trivial.

Added 1 commit:

• 61f75829 - reverting to original submission (no compact_optional)

since we don't want to expose the compact_optional, and there's no real benefit in just doing it internally, I've reverted the changes back to the original submission.

In any case, this will probably be supplanted by a more generic class once we sort out all the Conditions stuff.

Approved this merge request

Status changed to merged

mentioned in commit 0eb189a8

Resolved all discussions

mentioned in merge request atlas/Gaudi!99 (merged)

mentioned in commit atlas/Gaudi@d710ddcd

mentioned in commit atlas/Gaudi@1a46d24c