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

Allow types from different data models in interfaces #714

Merged
merged 12 commits into from
Dec 9, 2024

Conversation

m-fila
Copy link
Contributor

@m-fila m-fila commented Nov 29, 2024

BEGINRELEASENOTES

  • Added possibility to use in the interfaces the datatypes from different podio-based datamodels

ENDRELEASENOTES

The data types had to be aware of interfaces in which they were used in order to make them friend. For instance the types from base model couldn't be made aware of types in model extension so it was impossible to define a model extension with an interface that uses types from base model. #611

It seems the friend internal access was only used to get pointers to internal object in order to define operator< (for usage with maps) by comparing addresses.

In this PR I propose to remove the friend coupling of interfaces and data types. Instead a podio::detail::getRankfree function is added as a friend to each data type. The functions returns unsigned numeric type that can be used for comparisons. According to the standard the most appropriate type here - uintptr_t- is optional, if it's not present then uintmax_t will be used as it's the biggest unsigned type so a pointer will also fit in it.

I had hard time choosing a name for numeric value, that can be used for comparisons and I'm not entirely happy with calling it rank.

Another extension model is used as the extension we had had different getSyntax than the base model which the interfaces can't handle.

Renamed Rank to OrderKey

@tmadlener
Copy link
Collaborator

I think this is a nice solution to the problem. We will potentially need to put some comments into the code as well to explain why we have this (or mention it in some of the documentation).

One potential issue I see with this approach is that we are technically leaking the Obj* addresses to the outside, right? For example, it would be possible to do the following:

MutableHit hit;
auto rank = detail::getRank(hit);
auto hitObjPtr = reinterpret_cast<HitObj*>(rank);
// now we have access to the hitObjPtr!

I don't really see a way to avoid this though at the moment. We would need to have a way to have access to both ranks simultaneously without leaking the actual values to the outside. In the end we probably have to weigh whether this potential leak is worth the additional feature.

What other names did you have on your short list instead of rank?

@m-fila
Copy link
Contributor Author

m-fila commented Dec 2, 2024

Thanks

One potential issue I see with this approach is that we are technically leaking the Obj* addresses to the outside, right?

Yes, unfortunately this is possible. We could hide it in a proxy like this:

class Rank {
public:
  Rank(void* rank) : m_rank(reinterpret_cast<value_type>(rank)) {
  }
  friend bool operator<(const Rank& lhs, const Rank& rhs) {
    return lhs.m_rank < rhs.m_rank;
  }

private:
#ifdef UINTPTR_MAX
  using value_type = uintptr_t;
#else
  using value_type = uintmax_t;
#endif
  value_type m_rank;
};

What other names did you have on your short list instead of rank?

  • rankValue
  • ordinal, ordinalValue,
  • order, orderKey
  • priority

@tmadlener
Copy link
Collaborator

tmadlener commented Dec 2, 2024

How hard is it to implement the whole thing with the Rank class? If it allows for hiding the Obj* from outside access, I would be in favor of that.

order, orderKey

What about OrderKey as class name, instead of Rank? It describes pretty much what it is intended for and is still somewhat generic, I think.

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.

Nice, thanks. From my point of view there are currently only two smallish questions. But in general, I like this and the feature it enables.

include/podio/detail/OrderKey.h Outdated Show resolved Hide resolved
@@ -97,7 +97,7 @@
bool operator!=(const {{ inverse_type }}& other) const { return !(*this == other); }

// less comparison operator, so that objects can be e.g. stored in sets.
bool operator<(const {{ full_type }}& other) const { return m_obj < other.m_obj; }
bool operator<(const {{ full_type }}& other) const { return podio::detail::getOrderKey(*this) < podio::detail::getOrderKey(other); }
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is this strictly necessary? Wondering whether the compiler sees through all of this wrt optimizations.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

For datatypes this is not necessary, I just thought it could be easier to maintain if the same mechanism was used for both datatypes and interfaces. I'll have to take a look at the cost

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.

Stupid question that didn't occur to me before: I suppose I/O works as expected, but I don't see a test for that?

include/podio/detail/OrderKey.h Outdated Show resolved Hide resolved
@m-fila
Copy link
Contributor Author

m-fila commented Dec 5, 2024

Ups, I forgot about IO tests. For interfaces testing IO is just checking links and relations, right?

@tmadlener
Copy link
Collaborator

Yeah, basically the one place where it could break that is not covered by other tests is during relation resolving. So a minimal test would write some objects from the upstream model and some from the downstream model and use them to fill a relation where an interface is used.

I don't suspect for anything to go wrong, but I would sleep better if there was a test telling me that ;)

@m-fila m-fila force-pushed the cross_edm_interfaces branch from 64d5362 to adecb5f Compare December 9, 2024 08:35
@m-fila
Copy link
Contributor Author

m-fila commented Dec 9, 2024

Added IO tests

@m-fila m-fila requested a review from tmadlener December 9, 2024 12:41
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. Looks like nothing unexpected popped up :)

@tmadlener tmadlener merged commit 98a15af into AIDASoft:master Dec 9, 2024
18 checks passed
@m-fila m-fila deleted the cross_edm_interfaces branch December 9, 2024 13:31
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

Successfully merging this pull request may close these issues.

Use types of heterogeneous origins in interfaces
2 participants