Skip to content
New issue

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

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

Already on GitHub? # to your account

Remove the deprecated getters and setters from the generic parameters #415

Merged
merged 4 commits into from
May 15, 2023

Conversation

jmcarcell
Copy link
Member

BEGINRELEASENOTES

  • Remove the deprecated getters and setters from the generic parameters

ENDRELEASENOTES

It's the one year birthday this week of the commit that marked them as deprecated so it has been a long time that they have been deprecated and I don't think they are being used much if at all.

Copy link
Collaborator

@tmadlener tmadlener left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am wondering whether we should also remove the getXYZMap functions as they should only be used internally, but we can do that in another PR as well once we have verified we only use them internally.

@jmcarcell
Copy link
Member Author

Done, I checked and didn't find any places in the key4hep stack. This makes public the getMap() functions

return getMap<std::string>();
}

private:
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe we can keep them private and make the SIO parts that need them friends. Since they are not publicly used anywhere yet, it would be quite nice to not have them public if it is not necessary.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yap. I would very much prefer that too

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok now they are friends although I find it not very elegant with the #ifdefs and the extra include, unless there is an easier way of doing it

Comment on lines 15 to 17
#if PODIO_ENABLE_SIO
#include <sio/definitions.h>
#endif
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this should work by simply forward declare these unconditionally, I think?

namespace sio {
  class write_device;
  class read_device;
  using version_type = uint32_t; // from sio/definitions
}

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Now it's done that way

Copy link
Collaborator

@tmadlener tmadlener left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for also addressing the maps. For me this looks good now.

@tmadlener tmadlener merged commit e13b3e6 into AIDASoft:master May 15, 2023
@jmcarcell
Copy link
Member Author

This PR makes DD4hep not build for versions before 1.24; that is when the deprecated functions were removed

@tmadlener
Copy link
Collaborator

Maybe also make a PR to dd4hep itself to increas the min version of podio if it is not already high enough?

@jmcarcell jmcarcell deleted the deprecated branch May 16, 2023 08:42
@jmcarcell
Copy link
Member Author

It's 0.16, but the issue here is with older versions of dd4hep, develop has already removed the deprecated stuff

@tmadlener
Copy link
Collaborator

It looks like the getXYZMap have already escaped into the wild into the wild and are used in DD4hep.

https://github.com/AIDASoft/DD4hep/blob/11d0ab1e17e88ed393b83263db88b2e8f558f9f4/DDDigi/io/DigiIO.cpp#L352-L354

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

Successfully merging this pull request may close these issues.

3 participants