-
Notifications
You must be signed in to change notification settings - Fork 87
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
extended is_quantity<T> to support quantity-derived classes #286
Conversation
N.B. inheritance is preferred to composition/delegate class to avoid duplication and also inherent the various required operators ('+','-',..., '+=', ...) for details see discussion mpusz#271
src/core/include/units/quantity.h
Outdated
template<typename D, typename U, typename Rep> | ||
inline constexpr bool is_quantity<quantity<D, U, Rep>> = true; | ||
|
||
template<typename T> | ||
requires units::is_derived_from_specialization_of<T, units::quantity> && | ||
requires { | ||
typename T::dimension; | ||
typename T::unit; | ||
typename T::rep; | ||
requires Dimension<typename T::dimension>; | ||
requires Unit<typename T::unit>; | ||
requires Representation<typename T::rep>; } | ||
inline constexpr bool is_quantity<T> = true; | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you want types derived from quantity
to match the Quantity
concept probably all that is needed is:
template<typename T>
requires units::is_derived_from_specialization_of<T, units::quantity>
inline constexpr bool is_quantity<T> = true;
even removing the definition in lines 465-466.
However I am not sure if that solves your issue. See the next comment...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The rationale behind checking for inheritance is that the template class that derives from 'quantity' can have various different additional signatures and not a single static one. The unit-tests does not convey this properly because there is just the addition of one string literal.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
even removing the definition in lines 465-466.
The rationale behind these was to make the concept checks for the existence of the 'using' as early as possible and also to ensure that T
is a Representation
, has a valid Dimension
, and compatible Unit
.
Our use-case also works without but I saw in other parts of units that these are checked for... @mpusz if you could confirm that these lines should be removed.
Also: thanks a lot for your time in reviewing this PR! Much appreciated.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have to say I do not understand your answers.
What do you mean by:
the template class that derives from 'quantity' can have various different additional signatures and not a single static one
Do you mean a multiple inheritance here? Still a is_derived_from_specialization_of
should cover this. It covers the class that is the quantity
class template and all children that publicly derive from it. If you publically derive from it then dimension
, unit
, and rep
are always exposed unless you override them with a different type in a child class but I do not think we should check for such a case.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@mpusz you are absolutely right. I mixed up the actual is_derived_from_specialization_of<...>
with a similar version I saw/used elsewhere that had different internal workings. Will remove the additional requires
statement. Sorry for the confusion.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
incorporated requested change
/////////////////////////////////////// | ||
|
||
template<Representation Rep, units::Quantity Q, const units::basic_fixed_string additional_nttp_argument> | ||
struct derived_quantity : quantity<typename Q::dimension, typename Q::unit, Rep> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So we have:
derived_quantity<double, si::length<si::metre>, "a"> q1;
derived_quantity<double, si::length<si::kilometre>, "b"> q2;
auto q = q1 + q2;
The PR description suggests that the inheritance is selected over composition because one may want to reuse the operators. However q1 + q2
will return some kind of a quantity<...>
rather than derived_quantity<...>
which probably is not intended. As a result, all of the operators have to be redefined for derived_quantity
as well to properly handle the return type, right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[..] which probably is not intended
Actually yes, that was our intention ... at least that is in our use-case. As mentioned above, the unit-test is more functional and doesn't convey that aspect of the intention.
We'd primarily like to inherit the Quantity
concept and math operator overloads. The rest of the code would otherwise use standard 'quantity' definitions. I didn't want to make a specific extension that would work only/primarily for our use-case but tried to keep it a bit more generic for other users and/or our other uses-cases.
N.B. for what it's worth, a bit more detail on our use case: we add additional NTTP meta-information to the derived quantity template that are not directly linked to the quantity but rather to IO operations, static reflection ((de-)serialising), and external access rules (context: micro-services) such as:
- documenting the data struct member (ie. OpenAPI meta-infos),
- define external read/write access definition (e.g. internally non-const/constexpr -- since service needs to modify/recompute them -- but access from external function/users may be restricted),
- relationship between different member fields (ie. whether external access may modify individual fields, only in groups, or the whole data structure), and
- Role-Based-Access (RBAC) rules.
If you derive new quantities through, for example q1 + q2
, most of these IO meta-infos -- notably the variable 'description' -- become irrelevant until the final value in the processing chain ist assigned to a new 'Annotated' quantity that can be (de-)serialised.
Our present signature looks a bit like (assuming the new quantity extension):
template<units::Representation Rep, units::Unit U, const StringLiteral description = "",
const IoAccess direction = BOTH, const IoGroup group = NONE, const RbacRole = ALL>
struct Annotated : public units::quantity<U, Rep> {
/* [...] */
}
Most of these additional NTTPs are constexpr (or at least defined during compile-time) but may change their signature...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you derive new quantities through, for example
q1 + q2
, most of these IO meta-infos -- notably the variable 'description' -- become irrelevant until the final value in the processing chain ist assigned to a new 'Annotated' quantity that can be (de-)serialised.
Then why do you need Annotated
to model Quantity
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@JohelEGP Annotated
is basically a quantity
that is annotated for IO purposes. We need both the NTTPs as well as the mathematical operations that are defined for Quanitty
. Through inheritance one basically inherits a perfect operator forwarding rather than having to rewrite all operators/cases/exceptions by hand.
For example, the data flow in our microservice applications is typically:
A) net-IO generates and verifies a new input domain object -- ie. data class containing multiple Annotated<...>
fields,
B) multiple modules perform post-/internal-processing using the input domain objects and other regular quantity
, and
C) result is copied to an output domain object -- also containing multiple Annotated<...>
fields -- and serialised to IO.
In step 'A)' the additional NTTPs are used to validate the external user input (received via network), and in step 'C)' do verify and generate the OpenAPI documentation. We trying to keep the data and behaviour definitions together.
The 'Annotated` template helps us in this respect that all the relevant information has to be defined only once and in a single location (ie. domain object class/struct definition). The input/output validation is basically the conceptual extension of the 'units' physical unit safety concept to network IO-API safety...
Hope this helps. Let me know if you have further questions, suggestions or recommendations.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK. So the problem is that by defining the arithmetic and IO operators in terms on Quantity
makes it so that they don't work on Annotated
arguments, as opposed to parameters taking a specialization of quantity
, in which case Annotated
would convert to its base quantity
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Basically, yes. I tried without inheritance but then forwarding the operators became quite unwieldy.
From #123, I think a better approach to support types derived from Extending For perfert forwarding arguments of the form |
@JohelEGP I am not sure whether I understand your point correctly. The existing template<typename D, typename U, typename Rep>
inline constexpr bool is_quantity<quantity<D, U, Rep>> = true; which basically checks for the specific template parameter signature of template<typename T>
requires units::is_derived_from_specialization_of<T, units::quantity> && requires {
/* check for type consistency of T::Dimension, T::Unit, and T::Representation */ }
inline constexpr bool is_quantity<T> = true; while also performing some basic compatibility checks for the other depending types since the Is your argument referring to that
Could you elaborate? Thanks in advance.
With 'perfect forwarding', I intended to refer to that the derived class can re-use the same operators as its base definition (ie. |
Rather than changing Checking in the |
This PR allows to inherit from the
quantity
template class definition and to provide additional NTTP-type annotations and/or change the signature of the derived class (e.g. ordering of parameter types). For details see also #271N.B. inheritance is preferred to composition/delegate class to avoid duplication of and also inherent the various required operators ('+','-',..., '+=', ...)
Up for comments, improvements, and/or suggestions. Thanks again for this great library. 👍