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

Backport of some concepts from util to C++17 #1741

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

Conversation

gpicciuca
Copy link
Contributor

@gpicciuca gpicciuca commented Jan 31, 2025

Proposing a first bulk of changes aiming to backport some of the concepts used throughout the util module. (Not ready for merging)

There are some places where things are quite intricate and complicated, so feedback would be appreciated.

Problematic areas:

  • SerializeVector.h ends up with an incomplete type when building with C++17 flag
  • ConfigOptionProxy.h seems to not forward the underlying type correctly, causing the ValidatorFunction constraints to fail when using the addValidator method from ConfigOption.h (See commented code in BenchmarkExamples.cpp)
  • A solution has to be found for the isInstantiation constraints imposed in some parts of ConfigOption.h, which I currently had to remove to make it build with v3-ranges.

@gpicciuca gpicciuca force-pushed the cpp17-concepts-backport-2 branch from e50ec72 to 0949690 Compare January 31, 2025 10:03
Copy link

codecov bot commented Jan 31, 2025

Codecov Report

Attention: Patch coverage is 88.52459% with 7 lines in your changes missing coverage. Please review.

Project coverage is 89.95%. Comparing base (f2562fe) to head (d5d59b3).

Files with missing lines Patch % Lines
src/util/TypeTraits.h 0.00% 7 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #1741      +/-   ##
==========================================
- Coverage   89.95%   89.95%   -0.01%     
==========================================
  Files         395      395              
  Lines       37651    37664      +13     
  Branches     4234     4234              
==========================================
+ Hits        33869    33879      +10     
+ Misses       2487     2486       -1     
- Partials     1295     1299       +4     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Signed-off-by: Johannes Kalmbach <[email protected]>
@joka921
Copy link
Member

joka921 commented Jan 31, 2025

Thank you very much,
I played around a bit and learned some things.
For the future, please tell me, which targets you compiled (I assume benchmark among others), or whether you manually compiled even single header files etc. Otherwise I also get many errors of things that would have been easy to fix.

I update your branch, so make sure to git pull and have a look at my changes first:

  • General:
  1. Inside a CPP_template_def you have to use CPP_and_def (which I just implemented) instead of CPP_and
  2. Inside a CPP_template(_def) you have to use CPP_NOT(someConstraint) instead of !someConstraint.
    (I will also add those two points to the Wiki)
  • SerializeVector.h ends up with an incomplete type when building with C++17 flag
    I found some issues with the AD_SERIALIZE_FUNCTION_WITH_CONSTRAINT macros (in particular they currently don't support commas inside their arguments, I worked around for some of them, have a look and see if you can fix your problem in a similar way.
  • ConfigOptionProxy.h seems to not forward the underlying type correctly, causing the ValidatorFunction constraints to fail when using the addValidator method from ConfigOption.h (See commented code in BenchmarkExamples.cpp)
    This works now (the problem was, that std::invoke_result_t is not at all SFINAE-friendly, but still works with C++20 concepts. See my changes in TypeTraits.h for details.
  • A solution has to be found for the isInstantiation constraints imposed in some parts of ConfigOption.h, which I currently had to remove to make it build with v3-ranges.

I have fixed the isInstantiation and (as a proof of concept) readded it to the first occurence in ConfigOption.h, feel free to extend it to the remaining occurences.

I hope that with these changes and suggestions you can continue, please let me know when there is additional trouble, then we can communicate further (probably next week).

@gpicciuca
Copy link
Contributor Author

Thank you very much, I played around a bit and learned some things. For the future, please tell me, which targets you compiled (I assume benchmark among others), or whether you manually compiled even single header files etc. Otherwise I also get many errors of things that would have been easy to fix.

I update your branch, so make sure to git pull and have a look at my changes first:

  • General:
  1. Inside a CPP_template_def you have to use CPP_and_def (which I just implemented) instead of CPP_and
  2. Inside a CPP_template(_def) you have to use CPP_NOT(someConstraint) instead of !someConstraint.
    (I will also add those two points to the Wiki)
  • SerializeVector.h ends up with an incomplete type when building with C++17 flag
    I found some issues with the AD_SERIALIZE_FUNCTION_WITH_CONSTRAINT macros (in particular they currently don't support commas inside their arguments, I worked around for some of them, have a look and see if you can fix your problem in a similar way.
  • ConfigOptionProxy.h seems to not forward the underlying type correctly, causing the ValidatorFunction constraints to fail when using the addValidator method from ConfigOption.h (See commented code in BenchmarkExamples.cpp)
    This works now (the problem was, that std::invoke_result_t is not at all SFINAE-friendly, but still works with C++20 concepts. See my changes in TypeTraits.h for details.
  • A solution has to be found for the isInstantiation constraints imposed in some parts of ConfigOption.h, which I currently had to remove to make it build with v3-ranges.

I have fixed the isInstantiation and (as a proof of concept) readded it to the first occurence in ConfigOption.h, feel free to extend it to the remaining occurences.

I hope that with these changes and suggestions you can continue, please let me know when there is additional trouble, then we can communicate further (probably next week).

Thank you! For reference, I build the whole engine just to make sure that everything still builds especially since I'm currently working on the utility library which is used throughout a good portion of the codebase.

Will let you know if I spot more problems.

@sparql-conformance
Copy link

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