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

Extend requirement system to optional properties #117

Draft
wants to merge 7 commits into
base: master
Choose a base branch
from

Conversation

ThibaultLejemble
Copy link
Collaborator

This is a draft for #107.
The requires/provides system is updated to handle optional capabilities that can be checked at compile-time.
Enums are replaced by constexpr bool data members and the verification is done using SFINAE.

Things that change

  • a capability is declared using PROVIDES(CAPABILITY)
  • a capability is required by REQUIRES(CAPABILITY)
  • a macro IS_PROVIDED(CAPABILITY)) checks at compile-tile if a capability is provided or not by a base class
  • enums in the form PROVIDES_... are removed (they are now constexpr bool, and someone that uses myfit.PROVIDES_PLANE might still do the same since PROVIDES(PLANE) actually declares a data member named PROVIDES_PLANE)

Note that a class can still override a capability and no error or warning is printed.
I was thinking about printing at least a warning, but there are some capability such as NORMAL_DERIVATIVE that is provided by both MlsSphereFitDer and OrientedSphereDerImpl, but it is not really a programming mistake, one class overrides the capability and by design inherits the other class.

Potential next steps

  • document all the capabilities and generates the list of classes that provides the capabilities automatically (I am not 100% sure it is possible with doxygen)
  • extend the system with a macro like FIT(PLANE) that check if there is no other class that also fits the plane (in replacement to the run-time isValid() function)

Since enum {Check = PROVIDES_PLANE} is replaced by PROVIDES(PLANE), I changed all references of PROVIDES_PLANE to PLANE in the documentation.

@nmellado
Copy link
Contributor

Thanks for this very nice proposal.
I suggest to add the detection of duplicated classes in the hierarchy. My suggestion is to add the check in the declaration macro: the base class should not already have the same capability. As this feature is not always wanted (we only want it for container classes), maybe add PROVIDES_UNIQUE in addition to PROVIDES.

Does that make sense ?

@nmellado
Copy link
Contributor

Another question: could you demonstrate the use of the conditional capabilities usage ? (maybe a test of the macro IS_PROVIDED)

@ThibaultLejemble
Copy link
Collaborator Author

Thanks for this very nice proposal. I suggest to add the detection of duplicated classes in the hierarchy. My suggestion is to add the check in the declaration macro: the base class should not already have the same capability. As this feature is not always wanted (we only want it for container classes), maybe add PROVIDES_UNIQUE in addition to PROVIDES.

Does that make sense ?

The problem is that it is difficult to know in advance if a capability is unique or not.
When declaring a new primitive, or a new fitting operation, how to know a priori if some feature won't be overridden by another class, by another user?
For the current tests to pass, every capabilities can be provided by PROVIDES_UNIQUE except NORMAL_DERIVATIVE in OrientedSphereDerImpl that is overridden by MlsSphereFitDer.
Should I just remove the PROVIDES(NORMAL_DERIVATIVE) of MlsSphereFitDer (but keeping the method dNormal)?

@ThibaultLejemble
Copy link
Collaborator Author

Another question: could you demonstrate the use of the conditional capabilities usage ? (maybe a test of the macro IS_PROVIDED)

I added a small test for IS_PROVIDED : f8900b4

tests/src/capability.cpp Outdated Show resolved Hide resolved
tests/src/capability.cpp Show resolved Hide resolved
@nmellado
Copy link
Contributor

Thanks for this very nice proposal. I suggest to add the detection of duplicated classes in the hierarchy. My suggestion is to add the check in the declaration macro: the base class should not already have the same capability. As this feature is not always wanted (we only want it for container classes), maybe add PROVIDES_UNIQUE in addition to PROVIDES.
Does that make sense ?

The problem is that it is difficult to know in advance if a capability is unique or not. When declaring a new primitive, or a new fitting operation, how to know a priori if some feature won't be overridden by another class, by another user? For the current tests to pass, every capabilities can be provided by PROVIDES_UNIQUE except NORMAL_DERIVATIVE in OrientedSphereDerImpl that is overridden by MlsSphereFitDer. Should I just remove the PROVIDES(NORMAL_DERIVATIVE) of MlsSphereFitDer (but keeping the method dNormal)?

We need to take some time to discuss this AFK: some fitting extensions are Storage types, while others are Computation: some Computation object store their results in Storage types : this is a side effect, it must be unique in the Basket.

Copy link
Contributor

@nmellado nmellado left a comment

Choose a reason for hiding this comment

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

What do you think about highlighting storage-only capabilities, as suggested for curvature ?

{
PROVIDES_PRINCIPAL_CURVATURES
};
PROVIDES(PRINCIPAL_CURVATURES);
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
PROVIDES(PRINCIPAL_CURVATURES);
PROVIDES(PRINCIPAL_CURVATURES_STORAGE);

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.

2 participants