c++17 concepts with CPP_template for engine/idTable/ files #1746

sebastian-wieczorek · 2025-01-31T16:36:42Z

Backport C++20 concepts in the src/engine/idTable directory back to C++17 using macros from range-v3

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

joka921 · 2025-02-03T08:04:18Z

Hello @sebastian-wieczorek
Thank you very much for your work. I have just checked out your branch locally and brought it to compilation. In a separate step I will have a detailed look and address possible TODOs from your side.

For the initial declaration, in particular when a template is declared and in the same go defined in a header file, you have to use CPP_template. CPP_template_def is only for other redeclarations, or definitions that apppear after the initial declaration (typically in a .cpp file). The difference is, that the CPP_true parameter is defaulted to CPP_true = true in CPP_template and not in CPP_template_def (because redeclaring defaults is forbidden).
Each occurence of CPP_and must be on the same level as the requires keyword, not inside additional parentheses, so the following works CPP_template(typename T) (requires (3 < 5) CPP_and (4<7)) (parens around individual constraints), but the following does not: CPP_template(typename T) (requires (someThing<T> CPP_and someOtherThing<T>)) (because here the CPP_and is inside parens.
You have to use CPP_NOT, but you were already aware of this. I agree that the casing in the CPP_NOT is somewhat inconsistent and should probably be CPP_not, but that is in fact a macro from inside range-v3, where I want to keep the number of applied patches inside that external library as minimal as possible.

As already said, I have consistently applied points 1-3 and pushed your branch, the rest seems to work, but I will perform a separate review.

codecov · 2025-02-03T08:36:37Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 90.03%. Comparing base (f2562fe) to head (6043ba2).
Report is 18 commits behind head on master.

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #1746      +/-   ##
==========================================
+ Coverage   89.95%   90.03%   +0.07%     
==========================================
  Files         395      395              
  Lines       37651    37922     +271     
  Branches     4234     4264      +30     
==========================================
+ Hits        33869    34142     +273     
+ Misses       2487     2484       -3     
- Partials     1295     1296       +1

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

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

joka921

Thank you very much, I have addressed two of your TODOs myself (the solution was straightforward) , and left another one + some cleanup suggestions for you.
Please let me know if you have further questions of if I can be of assistance.
Best regards
Johannes

src/engine/idTable/IdTableRow.h

src/engine/idTable/CompressedExternalIdTable.h

joka921 · 2025-02-03T09:18:43Z

src/engine/idTable/IdTable.h

+  // TODO: <ccoecontrol> what about these? template cannot be defaulted
+  // possible solution: implement defaulted ones by hand


Thanks for pointing that out, In fact there seems to be no really simple + good C++-17 solution.
The manual implementation is rather error-prone, especially for the assignment.
I would suggest putting this whole block in its original form behind a #ifndef
So

#ifdef QLEVER_CPP_17 IdTable(const IdTable&) = default; IdTable& operator= (const IdTable&) = default; #else // The original code with the requires #endif ``` as soon as we compile in C++20 mode we will detect all the illegal copies. And additionally write a comment tha4t there is no simple way to formulate this in C++17 . (For completeness: We could add a separate layer of wrapper classes that only handles the viewness, but that would be overkill for now).

default put behind a macro here

joka921 · 2025-02-03T09:22:27Z

src/engine/idTable/IdTable.h

@@ -346,7 +365,8 @@ class IdTable {
  // Get a reference to the `i`-th row. The returned proxy objects can be
  // implicitly and trivially converted to `row_reference`. For the design
  // rationale behind those proxy types see above for their definition.
-  row_reference_restricted operator[](size_t index) requires(!isView) {
+  CPP_template(typename = void)(requires(!isView)) row_reference_restricted


This pattern happens very often, maybe wriite two macros at the beginning of this fiile
define REQUIRES_VIEW CPP_template(typename = void) (requires isView)
And the same for REQUIRES_NOT_VIEW, then you can write
REQUIRES_NOT_VIEW row_reference_restricted operator[](size_t index) here (similar in the other places),
whiich is nicer and uses less space.
Don't forget to undefine the macros again at the end of the header file to not clutter the global namespace.

my guess is that this will be more common that just this file, what about enhancing concepts.h with some macro like:
CPP_parameterless_template(...) CPP_template(__VA_ARGS__)

Something like that could work, I will look into it, as I have discovered some additional macros to be missing.

For this PR (to get it merged) we will leave the typename = void defaults,
But if I have time I will add some additional macros.

joka921 · 2025-02-03T09:24:40Z

src/engine/idTable/IdTable.h

+  // TODO: <ccoecontrol> what with this one?
+  // reimplementing it with CPP_template macro causes template redefinition
+  // errors


I'll first have a look at the remainder, and then think of something.

joka921 · 2025-02-03T09:35:20Z

src/engine/idTable/IdTable.h

+  CPP_template(typename = void)(
+      requires(isCloneable)) auto moveOrClone() const& {
+    return clone();
+  }
+  CPP_template(typename = void)(
+      requires(isCloneable)) IdTable&& moveOrClone() && {


This could just be rewritten. Probably when trying out you misplaced some of the many &&s here or something else went wrong.

yeah, I tried this with gcc13 and this was just showing errors from other parts of the code when compiled with backports on

here is gcc13 fix related to CPP_template usage for overloads or specializations, please share if you think is is needed.

That fix is definitely needed, I also have it in some open PRs... GCC11 didn't complain about this, but Clang16 (where I treid it) and obviously GCC13 (from what you say) correctly reject that code. It is time that we merge it in.

Co-authored-by: Johannes Kalmbach <[email protected]>

sebastian-wieczorek · 2025-02-06T16:27:50Z

@joka921 please share what do you think about this.

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

joka921

We can leave this as is for now,
We have learned something for the next iteration.

I think for member functions that are constrained on the template arguments of the outer template, the better alternative to CPP_template<typename = void> is
using the CPP_member macro in combination with CPP_ret as it yields the best code for both C++17 and C++20. I have incorporated those into some of the places.

sparql-conformance · 2025-02-10T10:47:40Z

Conformance check passed ✅

No test result changes.

Details: https://qlever.cs.uni-freiburg.de/sparql-conformance-ui?cur=6043ba2594963cffca03f7349db5303ca3b2628a&prev=551288e03ea264e60a67667c295a66c596cc6e50

sonarqubecloud · 2025-02-10T13:53:56Z

Quality Gate passed

Issues
6 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

c++17 concepts with CPP_template for engine/idTable/ files

dbf0b40

sebastian-wieczorek force-pushed the concepts_for_cpp17 branch from c317040 to dbf0b40 Compare January 31, 2025 16:48

Fix the usages of CPP_template(_def)

d67df46

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

Address two of the TODOs

f904bdd

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

joka921 requested changes Feb 3, 2025

View reviewed changes

sebastian-wieczorek and others added 4 commits February 6, 2025 13:13

Update src/engine/idTable/CompressedExternalIdTable.h

d3e034d

Co-authored-by: Johannes Kalmbach <[email protected]>

concept moved into anonymous namespace

66189d1

dealing with default thingies

d41a14b

gcc13 fixes related to CPP_template usage

2dba572

joka921 added 4 commits February 10, 2025 09:45

Address the remaining issues, so that this becomes ready for merge.

dd64d9d

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

some additional small changes

2cb1621

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

Additional experiments with range-v3 macros.

1667812

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

Fix the compilation.

6043ba2

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

joka921 approved these changes Feb 10, 2025

View reviewed changes

joka921 merged commit 5caf9f4 into ad-freiburg:master Feb 10, 2025
22 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

c++17 concepts with CPP_template for engine/idTable/ files #1746

c++17 concepts with CPP_template for engine/idTable/ files #1746

sebastian-wieczorek commented Jan 31, 2025 •

edited by joka921

Loading

joka921 commented Feb 3, 2025

codecov bot commented Feb 3, 2025 •

edited

Loading

joka921 left a comment

joka921 Feb 3, 2025

sebastian-wieczorek Feb 6, 2025

joka921 Feb 3, 2025

sebastian-wieczorek Feb 6, 2025 •

edited

Loading

joka921 Feb 10, 2025

joka921 Feb 10, 2025

joka921 Feb 3, 2025

joka921 Feb 3, 2025

sebastian-wieczorek Feb 6, 2025

sebastian-wieczorek Feb 6, 2025 •

edited

Loading

joka921 Feb 10, 2025

sebastian-wieczorek commented Feb 6, 2025

joka921 left a comment

sparql-conformance bot commented Feb 10, 2025

sonarqubecloud bot commented Feb 10, 2025

		// TODO: <ccoecontrol> what about these? template cannot be defaulted
		// possible solution: implement defaulted ones by hand

c++17 concepts with CPP_template for engine/idTable/ files #1746

c++17 concepts with CPP_template for engine/idTable/ files #1746

Conversation

sebastian-wieczorek commented Jan 31, 2025 • edited by joka921 Loading

joka921 commented Feb 3, 2025

codecov bot commented Feb 3, 2025 • edited Loading

Codecov Report

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

sebastian-wieczorek Feb 6, 2025 • edited Loading

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

sebastian-wieczorek Feb 6, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sebastian-wieczorek commented Feb 6, 2025

joka921 left a comment

Choose a reason for hiding this comment

sparql-conformance bot commented Feb 10, 2025

Conformance check passed ✅

sonarqubecloud bot commented Feb 10, 2025

Quality Gate passed

sebastian-wieczorek commented Jan 31, 2025 •

edited by joka921

Loading

codecov bot commented Feb 3, 2025 •

edited

Loading

sebastian-wieczorek Feb 6, 2025 •

edited

Loading

sebastian-wieczorek Feb 6, 2025 •

edited

Loading