Backport C++20 concepts to C++17 in the 'sparqlExpressions' module #1787

krzysztof-tyb · 2025-02-10T16:33:53Z

The backport is implemented using macros from Eric Niebler's range-v3 library.

krzysztof-tyb · 2025-02-10T16:37:14Z

Here is the sparqlExpressions part of backporting concepts to C++17.
I didn't find a better solution for concepts used in template lambda. For this reason, I used the CPP_assert macro to handle them inside lambda.

krzysztof-tyb · 2025-02-11T12:17:54Z

src/engine/sparqlExpressions/SparqlExpressionGenerators.h

+                                       ValueGetter&& valueGetter) {
+  CPP_assert(SingleExpressionResult<Input>);
+  auto transformation = [context, valueGetter]<typename I>(I&& i)
+      -> CPP_ret(decltype(valueGetter(AD_FWD(i), context)))(


For some reason, clang17 and older cause the compiler to crash if the return type is explicitly defined in nested lambda.
Clang18 compiles it correctly.
It looks like a bug in older compilator version.

krzysztof-tyb · 2025-02-12T13:17:12Z

src/engine/sparqlExpressions/SparqlExpressionGenerators.h

-  auto transformation = [context, valueGetter]<typename I>(I&& i)
-      -> CPP_ret(decltype(valueGetter(AD_FWD(i), context)))(
-          requires ranges::invocable<ValueGetter, I&&, EvaluationContext*>) {
+inline auto valueGetterGenerator = CPP_lambda()(


@joka921
To solve the clang crash for lambdas mentioned previously I added the CPP_lambda macro which expands to requires in C++20 and std::enable_if_t (as additional lambda argument) in C++17.
What do you think about it? It can be used instead of CPP_assert in other lambdas.

Yes, I like the CPP_lambda macro, feel free to use it in the other places,
but the CPP_asserts are also fine in the many places where there is no overload resolution happening.

joka921

Very nice, I have some suggestions and some open questions, partly also for myself.
I suggest that you read my comments and make another pass with the things you think are appropriate, and then you ping me again and I make a final pass with the small cleanups and the merging together etc.

joka921 · 2025-02-12T13:24:50Z

src/backports/concepts.h

+#define CPP_LAMBDA_AUX_WHICH_(FIRST, ...) CPP_PP_EVAL(CPP_PP_CHECK, FIRST)
+
+#define CPP_LAMBDA_AUX_0(...) __VA_ARGS__
+


I like the idea very much, I also have added some additional macros in another branch and will consolidate them when merging (in particular I have opened a several file for the internal SFINAE_AUX_WHICH_BLA_... macros.

joka921 · 2025-02-12T13:30:51Z

src/engine/GroupBy.cpp

+            CPP_assert(sparqlExpression::SingleExpressionResult<T> &&
+                       VectorOfAggregationData<A>);


I would suggest two separate CPP_assert() for &&, because then you get better diagnostics in the failure case.

joka921 · 2025-02-12T13:31:57Z

src/engine/LazyGroupBy.cpp

-         &evaluationContext]<sparqlExpression::SingleExpressionResult T>(
+        [blockSize, &evaluationContext]<typename T>(
            VectorOfAggregationData auto& aggregateData, T&& singleResult) {
+          CPP_assert(sparqlExpression::SingleExpressionResult<T>);


Now that you have it, you can consider rewriting the CPP-asser with CPP_lambda ,but I am also fine with the asserts in places where we don't need overload resolution. Just let me know how you decide.

I decided to use CPP_lambda instead of CPP_assert but not all. I keep CPP_assert where we have template parameter packs at this moment. We need to somehow deal with the lambda explicit template parameter list to be compatible with C++17.

joka921 · 2025-02-12T13:39:06Z

src/engine/sparqlExpressions/AggregateExpression.cpp

+      if constexpr (ranges::invocable<decltype(aggregateOperation._function),
+                                      decltype(AD_FWD(x)),
+                                      decltype(AD_FWD(y))>) {


If you give the X and Y names in the lambda declaration, then this is
ranges::invocable<AggregateOperation, X&&, Y&&>

This is more readable then.

Do you mean X, and Y as lambda template parameters?
However, the problem is that the lambda explicit template parameter list has been available since C++20.
So later we need to somehow deal with this to be compatible with C++17.

src/engine/sparqlExpressions/AggregateExpression.h

joka921 · 2025-02-12T14:08:41Z

src/engine/sparqlExpressions/SparqlExpressionGenerators.h

-  auto transformation = [context, valueGetter]<typename I>(I&& i)
-      -> CPP_ret(decltype(valueGetter(AD_FWD(i), context)))(
-          requires ranges::invocable<ValueGetter, I&&, EvaluationContext*>) {
+inline auto valueGetterGenerator = CPP_lambda()(


Yes, I like the CPP_lambda macro, feel free to use it in the other places,
but the CPP_asserts are also fine in the many places where there is no overload resolution happening.

src/engine/sparqlExpressions/SparqlExpressionGenerators.h

src/engine/sparqlExpressions/SparqlExpressionTypes.h

joka921 · 2025-02-12T14:23:20Z

src/util/TypeTraits.h

+
+// Ensures that T is a floating-point type.
+template <typename T>
+CPP_concept FloatingPoint = std::is_floating_point_v<T>;


I still have to figure out where to put them and what to name them.

I think I already have a file src/backports/concepts.h where I want a common namespace for the concepts that are in C++20.

codecov · 2025-02-12T15:45:24Z

Codecov Report

Attention: Patch coverage is 96.68508% with 6 lines in your changes missing coverage. Please review.

Project coverage is 90.03%. Comparing base (1570033) to head (5251f9e).
Report is 2 commits behind head on master.

Files with missing lines	Patch %	Lines
src/engine/Filter.cpp	89.74%	0 Missing and 4 partials ⚠️
...engine/sparqlExpressions/RelationalExpressions.cpp	75.00%	1 Missing ⚠️
...ine/sparqlExpressions/SparqlExpressionGenerators.h	96.77%	0 Missing and 1 partial ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #1787      +/-   ##
==========================================
+ Coverage   90.02%   90.03%   +0.01%     
==========================================
  Files         396      396              
  Lines       37974    37989      +15     
  Branches     4262     4262              
==========================================
+ Hits        34185    34203      +18     
+ Misses       2493     2491       -2     
+ Partials     1296     1295       -1

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

krzysztof-tyb · 2025-02-13T14:21:44Z

@joka921 Thank you for your feedback!
I've added some modifications according to your comments.
Please let me know if you have more suggestions.

…essions' into cxx17_concepts_sparqlExpressions # Conflicts: # src/backports/concepts.h # src/engine/Bind.cpp # src/engine/Filter.cpp # src/engine/GroupBy.cpp # src/engine/LazyGroupBy.cpp # src/engine/sparqlExpressions/AggregateExpression.h # src/engine/sparqlExpressions/ConditionalExpressions.cpp # src/engine/sparqlExpressions/SparqlExpressionGenerators.h # src/engine/sparqlExpressions/SparqlExpressionTypes.h # src/engine/sparqlExpressions/StringExpressions.cpp

# Conflicts: # src/backports/concepts.h

Signed-off-by: Johannes Kalmbach <[email protected]>

joka921

Thank you very much.
I have merged this with the master, make sure to git pull before continuing.
We now have the following additional macro:
CPP_template_lambda(captures)(typename T)(T&& arg) (requires SingleExpressionResult<T>),
Please consistently use it for the places where it helps (I have marked them).

Also add the missing macro CPP_template_lambda_mut (the same but with mutable),
and use it in the places I marked accordingly.

For the macro implementation, please follow the style I now chose: The internal BLA_BLAJ_AUX_WHICH_ macros come into the internal CPPtemplate2.h file, and only the final exposition + the documentation is done in the user-facing concepts.h folder.

That should then be the final iteration for this PR.
Let me know if I can be of additional assistance.

src/backports/concepts.h

joka921 · 2025-02-14T08:01:17Z

src/engine/Bind.cpp

-    constexpr static bool isVariable = std::is_same_v<T, ::Variable>;
-    constexpr static bool isStrongId = std::is_same_v<T, Id>;
+  auto visitor = CPP_lambda_mut(&)(auto&& singleResult)(
+      requires sparqlExpression::SingleExpressionResult<


The deacy_t is not needed, and maybe CPP_template_lambda_mut

At least get rid of all the decltype(singleResult) below.

joka921 · 2025-02-14T08:02:23Z

src/engine/Filter.cpp

+              std::decay_t<decltype(singleResult)>>) {
+    if constexpr (std::is_same_v<std::decay_t<decltype(singleResult)>,
+                                 ad_utility::SetOfIntervals>) {


CPP_template_lambda

joka921 · 2025-02-14T08:03:22Z

src/engine/GroupBy.cpp

+  auto visitor = CPP_lambda_mut(&)(auto&& singleResult)(
+      requires sparqlExpression::SingleExpressionResult<
+          std::decay_t<decltype(singleResult)>>) {
+    constexpr static bool isStrongId =


CPP_template_lambda_mut

then replace all the decltype(...)s below.

joka921 · 2025-02-14T08:03:51Z

src/engine/GroupBy.cpp

-                  &outCol]<sparqlExpression::SingleExpressionResult T>(
-                     T&& singleResult) mutable {
+  auto visitor = CPP_lambda_mut(&evaluationContext, &resultTable, &localVocab,
+                                &outCol)(auto&& singleResult)(


joka921 · 2025-02-14T08:07:45Z

src/engine/sparqlExpressions/ConditionalExpressions.cpp

+    auto visitConstantExpressionResult = CPP_lambda(
+        &nextUnboundIndices, &unboundIndices, &isUnbound, &result,
+        ctx)(auto&& childResult)(
+        requires SingleExpressionResult<std::decay_t<decltype(childResult)>> &&
+        isConstantResult<std::decay_t<decltype(childResult)>>) {


CPP_template_lambda

joka921 · 2025-02-14T08:08:03Z

src/engine/sparqlExpressions/ConditionalExpressions.cpp

+        CPP_lambda(&result, &unboundIndices, &nextUnboundIndices, &ctx,
+                   &isUnbound)(auto&& childResult)(requires CPP_NOT(
+            isConstantResult<std::decay_t<decltype(childResult)>> &&
+            SingleExpressionResult<std::decay_t<decltype(childResult)>> &&
+            std::is_rvalue_reference_v<decltype(childResult)>)) {


CPP_template_lambda

joka921 · 2025-02-14T08:08:31Z

src/engine/sparqlExpressions/ConditionalExpressions.cpp

+    auto visitExpressionResult = CPP_lambda(
+        &visitConstantExpressionResult,
+        &visitVectorExpressionResult)(auto&& childResult)(
+        requires SingleExpressionResult<std::decay_t<decltype(childResult)>> &&


CPP_template_lambda

joka921 · 2025-02-14T08:15:25Z

src/engine/sparqlExpressions/SparqlExpressionGenerators.h

+    CPP_lambda()(size_t numElements, EvaluationContext* context, auto&& input,
+                 auto&& valueGetter)(
+        requires SingleExpressionResult<std::decay_t<decltype(input)>>) {
+  auto transformation = CPP_lambda(context, valueGetter)(auto&& i)(
+      requires ranges::invocable<std::decay_t<decltype(valueGetter)>,


CPP_template_lambda

joka921 · 2025-02-14T08:21:29Z

src/engine/sparqlExpressions/StringExpressions.cpp

-            requires std::is_rvalue_reference_v<T&&> {
-      if constexpr (isConstantResult<T>) {
+    auto visitSingleExpressionResult = CPP_lambda(&ctx, &result)(auto&& s)(
+        requires SingleExpressionResult<std::decay_t<decltype(s)>> &&


CPP_template_lambda

joka921 · 2025-02-14T08:34:14Z

I now see your Comment about []<typename T>(){} being technically C++20.
That is technically true, but we have discovered that your targeted toolchain (Gcc8.3/Qcc 8.3) supports them without warning in C++17 mode, so I am currently assuming that that is fine.
I would suggest the following: Use my suggestions to use this feature to make the code readable.
If at some point we find out that that is a problem because of compliance etc, then it will be my fault, and I will have to find a solution on my time, as I have guided you on that path.
Does that work for you?

krzysztof-tyb · 2025-02-14T13:36:08Z

I used CPP_template_lambda macros as you suggested.
I think we can leave these template lambda features as they are now. In future, if they will cause a problem, we will think about it.
Thank you for your support with this PR!

sparql-conformance · 2025-02-14T13:59:54Z

Conformance check passed ✅

No test result changes.

Details: https://qlever.cs.uni-freiburg.de/sparql-conformance-ui?cur=5251f9ee86b15ed8fdf75016242e3e856a126cdc&prev=1570033d07eb625dd3c2624c866eeb241f8639ef

sonarqubecloud · 2025-02-14T15:23:22Z

Quality Gate passed

Issues
9 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

joka921

Thank you very much, this looks nice and clean now.

joka921

Thank you, that looks great now

Backport C++20 concepts to C++17 in the 'sparqlExpressions' module

214825f

Fix CPP_assert for valueGetterGenerator

707f5bf

krzysztof-tyb commented Feb 11, 2025

View reviewed changes

Add CPP_lambda macro

90b077e

krzysztof-tyb commented Feb 12, 2025

View reviewed changes

joka921 requested changes Feb 12, 2025

View reviewed changes

krzysztof-tyb added 5 commits February 13, 2025 13:55

Backport C++20 concepts to C++17 in the 'sparqlExpressions' module

9f27715

Fix CPP_assert for valueGetterGenerator

eca25a0

Add CPP_lambda macro

5ad8aff

Add CPP_lambda_mut macro

9211c2b

Modifications according to comments

3c6751b

krzysztof-tyb force-pushed the cxx17_concepts_sparqlExpressions branch from 90b077e to 3c6751b Compare February 13, 2025 13:40

joka921 added 3 commits February 14, 2025 08:48

Merge branch 'master' into cxx17_concepts_sparqlExpressions

ed92968

# Conflicts: # src/backports/concepts.h

Fix the lambdas

a61c2fe

Signed-off-by: Johannes Kalmbach <[email protected]>

joka921 requested changes Feb 14, 2025

View reviewed changes

Use CPP_template_lambda and CPP_template_lambda_mut

5251f9e

joka921 approved these changes Feb 14, 2025

View reviewed changes

joka921 merged commit 2d6046a into ad-freiburg:master Feb 14, 2025
23 of 24 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Backport C++20 concepts to C++17 in the 'sparqlExpressions' module #1787

Backport C++20 concepts to C++17 in the 'sparqlExpressions' module #1787

krzysztof-tyb commented Feb 10, 2025 •

edited by joka921

Loading

krzysztof-tyb commented Feb 10, 2025

krzysztof-tyb Feb 11, 2025

krzysztof-tyb Feb 12, 2025 •

edited

Loading

joka921 Feb 12, 2025

joka921 left a comment

joka921 Feb 12, 2025

joka921 Feb 12, 2025

joka921 Feb 12, 2025

krzysztof-tyb Feb 13, 2025

joka921 Feb 12, 2025

krzysztof-tyb Feb 13, 2025

joka921 Feb 12, 2025

joka921 Feb 12, 2025

codecov bot commented Feb 12, 2025 •

edited

Loading

krzysztof-tyb commented Feb 13, 2025

joka921 left a comment

joka921 Feb 14, 2025

joka921 Feb 14, 2025

joka921 Feb 14, 2025

joka921 Feb 14, 2025

joka921 Feb 14, 2025

joka921 Feb 14, 2025

joka921 Feb 14, 2025

joka921 Feb 14, 2025

joka921 Feb 14, 2025

joka921 Feb 14, 2025

joka921 Feb 14, 2025

joka921 commented Feb 14, 2025

krzysztof-tyb commented Feb 14, 2025

sparql-conformance bot commented Feb 14, 2025

sonarqubecloud bot commented Feb 14, 2025

joka921 left a comment

joka921 left a comment

		#define CPP_LAMBDA_AUX_WHICH_(FIRST, ...) CPP_PP_EVAL(CPP_PP_CHECK, FIRST)

		#define CPP_LAMBDA_AUX_0(...) __VA_ARGS__

		CPP_assert(sparqlExpression::SingleExpressionResult<T> &&
		VectorOfAggregationData<A>);

Backport C++20 concepts to C++17 in the 'sparqlExpressions' module #1787

Backport C++20 concepts to C++17 in the 'sparqlExpressions' module #1787

Conversation

krzysztof-tyb commented Feb 10, 2025 • edited by joka921 Loading

krzysztof-tyb commented Feb 10, 2025

Choose a reason for hiding this comment

krzysztof-tyb Feb 12, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

joka921 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Feb 12, 2025 • edited Loading

Codecov Report

krzysztof-tyb commented Feb 13, 2025

joka921 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

joka921 commented Feb 14, 2025

krzysztof-tyb commented Feb 14, 2025

sparql-conformance bot commented Feb 14, 2025

Conformance check passed ✅

sonarqubecloud bot commented Feb 14, 2025

Quality Gate passed

joka921 left a comment

Choose a reason for hiding this comment

joka921 left a comment

Choose a reason for hiding this comment

krzysztof-tyb commented Feb 10, 2025 •

edited by joka921

Loading

krzysztof-tyb Feb 12, 2025 •

edited

Loading

codecov bot commented Feb 12, 2025 •

edited

Loading