Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Proposal to make the Const classes default and explicitly mark the mutable ones as mutable #204

Closed
tmadlener opened this issue Aug 10, 2021 · 1 comment

Comments

@tmadlener
Copy link
Collaborator

tmadlener commented Aug 10, 2021

This issue is mainly intended to gather feedback from users we are potentially not yet aware of. During the discussion at the EDM4hep meeting on Aug 10 there was general agreement that the following proposal should be adopted. However, we also agreed that we would still like to collect more feedback as it constitutes a breaking change. Additionally, it should also help us with possible migration strategies.

Please let us know what you think

TL;DR (reasoning below)

The proposal is to make the Const user-facing classes that are generated by podio the "default" classes and rename the current "default" classes to something that explicitly marks them as mutable. Either via

  • putting these classes into a separate namespace (e.g. mut or mod)
  • or by sticking a Mutable (or Mut) to the class name as pre- or post-fix

The main reason for this proposal is that it makes the default use case of simply reading data easier to write, and additionally making it very clear when something needs to be mutated. The details of the naming scheme are still part of the discussion and input is welcome.

Current situation in podio

For the example datatype

MCParticle:
  Members:
    - int pdg // PDG

podio generates the following user classes at the moment

struct ConstMCParticle {
  const int& getPdg() const;
};

struct MCParticle {
  void setPdg(int);
  const int& getPdg() const;
  operator ConstMCParticle() const; // conversion to Const
};

The only difference between the two classes is that the Const version offers only getter functions, but no setter functions. The collection interface looks something like this (just showing enough to make the point)

struct MCParticleConstCollectionIterator {
  ConstMCParticle operator*();
};

struct MCParticleCollectionIterator {
  MCParticle operator*();
};

struct MCParticleCollection {
  MCParticle operator[](unsigned);
  ConstMCParticle operator[](unsigned) const;
  
  MCParticleCollectionIterator begin();
  MCParticleConstCollectionIterator begin() const;
};

Again a duplication of the classes is necessary here to make the interface const-correct; Depending on whether the collection is const (in the c++ sense) access to the Const (in the podio sense) or the "normal" classes is granted.

Problems / Issues with the current approach

  • The presence of the Const classes is not documented (Document Const* Interfaces #198) even though they should be the default use case for reading
  • The default use case should be the "easier one to use"
  • There is no implicit conversion from the "default" class to the Const class. This is by design as these conversions should only make the access more strict.

Where users might run into problems with the current approach is when writing interfaces for podio generated EDMs, e.g.

void foo(const MCParticle& p); // const-ref is a slightly unusual construct with value semantics
void bar(MCParticle p); // One would expect that this works

const auto& coll = store.get<MCParticleCollection>("mcs");
auto p = coll[0];
foo(p); // error: invalid initialization of reference of type 'const MCParticle&' from expression of
        //  type 'ConstMCParticle'
bar(p); // error: could not convert 'p' from 'ConstMCParticle' to 'MCParticle'

Here users actually need to know about the presence of the Const in order to write interfaces that only need reading access. This makes it unnecessarily complicated to write an interface. It would become simpler if the "default" classes are immutable and mutable access requires "more work".

@tmadlener
Copy link
Collaborator Author

Closed via #205

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

1 participant