Backport src/engine/sparqlExpressions/ to C++17 #2471
Conversation
This commit includes C++17 compatibility fixes for the SPARQL expressions module: - Use inline constexpr variables instead of C++20 consteval - Replace template auto parameters with explicit template parameters where needed - Add [[maybe_unused]] attribute using QL_MAYBE_UNUSED macro - Use std::invoke for member function pointers and function objects - Replace direct lambda capture of constexpr variables with references - Add missing #ifndef guards for unreachable code paths in switch statements - Update template syntax for better C++17 compatibility - Use explicit instantiation for complex template types 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>
joka921
left a comment
joka921
left a comment
There was a problem hiding this comment.
A round of self-reviews.
| if constexpr (std::is_constructible_v<R, decltype(AD_FWD(x))>) { | ||
| if constexpr (std::is_constructible_v<R, decltype(AD_FWD(x))> && | ||
| !ad_utility::similarToInstantiation<R, | ||
| VectorWithMemoryLimit>) { |
There was a problem hiding this comment.
Have a look at what is going on here (it has to do with the expression types and the private/publicness of the clone method.
There was a problem hiding this comment.
Maybe postpone this to another PR, as it is nontrivial, and connected to other things.
| StringExpressionImpl<1, Hash<decltype(ad_utility::hashMd5)>>; | ||
| using SHA1Expression = | ||
| StringExpressionImpl<1, decltype(hash<ad_utility::hashSha1>)>; | ||
| StringExpressionImpl<1, Hash<decltype(ad_utility::hashSha1)>>; | ||
| using SHA256Expression = | ||
| StringExpressionImpl<1, decltype(hash<ad_utility::hashSha256>)>; | ||
| StringExpressionImpl<1, Hash<decltype(ad_utility::hashSha256)>>; | ||
| using SHA384Expression = | ||
| StringExpressionImpl<1, decltype(hash<ad_utility::hashSha384>)>; | ||
| StringExpressionImpl<1, Hash<decltype(ad_utility::hashSha384)>>; | ||
| using SHA512Expression = | ||
| StringExpressionImpl<1, decltype(hash<ad_utility::hashSha512>)>; | ||
| StringExpressionImpl<1, Hash<decltype(ad_utility::hashSha512)>>; |
There was a problem hiding this comment.
I think we can get rid of the decltype and then also of the inline variables in the header.
There was a problem hiding this comment.
No, those are fine for now.
Signed-off-by: Johannes Kalmbach <[email protected]>
| #define INSTANTIATE_AGG_EXP(Function, ValueGetter) \ | ||
| template class AggregateExpression< \ | ||
| Operation<2, FunctionAndValueGetters<Function, ValueGetter>>>; | ||
| INSTANTIATE_AGG_EXP(decltype(addForSum), NumericValueGetter); |
There was a problem hiding this comment.
Is there a reason that this decltype(addForSum) is not rewritten like the other cases below?
| }; | ||
| using AvgOperation = | ||
| Operation<2, | ||
| FunctionAndValueGetters<decltype(addForSum), NumericValueGetter>>; |
| [[nodiscard]] VectorWithMemoryLimit clone() const { | ||
| // Call the private copy constructor. | ||
| return VectorWithMemoryLimit(*this); | ||
| return VectorWithMemoryLimit(CloneTag{}, *this); |
There was a problem hiding this comment.
could you please explain what is going on here, in particular what the purpose of the CloneTag struct is?
Signed-off-by: Johannes Kalmbach <[email protected]>
Signed-off-by: Johannes Kalmbach <[email protected]>
ullingerc
left a comment
ullingerc
left a comment
There was a problem hiding this comment.
If the checks run through now, I'm happy with this.
Overview
Conformance check passed ✅No test result changes. |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #2471 +/- ##
==========================================
- Coverage 91.07% 91.06% -0.02%
==========================================
Files 466 466
Lines 39706 39691 -15
Branches 5309 5309
==========================================
- Hits 36164 36145 -19
- Misses 2034 2035 +1
- Partials 1508 1511 +3 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
2 participants
This commit includes C++17 compatibility fixes for the SPARQL expressions module:
🤖 Generated with Claude Code