migrate to C++20 - #507
Merged
Merged
Conversation
Reviewer's GuideMigrates the project and tests to C++20 and modernizes several utilities and templates to use C++20 features (concepts/requires, std::erase_if, std::remove_cvref_t, std::source_location, etc.), while tightening some APIs and fixing minor portability/constexpr issues. Class diagram for updated C++20 utility types and helpersclassDiagram
class Vector {
+Vector()
+Vector(Base &&x)
+Base::const_iterator begin() const
+Base::const_iterator end() const
+bool empty() const
+size_t size() const
+const Value &at(size_t pos) const
+const Value &operator[](size_t pos) const
}
class Vector_Base {
<<typedef>>
}
Vector_Base <.. Vector : Base
class Array_T_N {
T[N] data
+Array(First &&first, Types &&...args)
}
class std_array_T_N {
}
Array_T_N --|> std_array_T_N
class TaggedInt_underlying_helper_T {
<<template>>
+type
}
class AnsiOstream {
-std::ostream &m_os
+write(char16_t codepoint)
+write(char32_t codepoint)
+write(std::string_view sv)
+write(std::u8string_view sv)
+write(T n)
}
class MakeQPointer {
<<function template>>
+makeQPointer(T, Args...)
}
class QObject {
}
class QPointer_T {
}
MakeQPointer ..> QPointer_T : returns
MakeQPointer ..> QObject : requires base of
class numeric_hash_fn {
+size_t numeric_hash(std::integral auto val)
}
class mm_source_location_alias {
<<using>>
+source_location = std::source_location
}
class FloatPredicates {
+constexpr bool isNan(FloatType f)
+constexpr bool isFinite(FloatType f)
}
class Utils_templates {
+bool listRemoveIf(std::list<T> &list, Predicate should_remove)
+remove_cvref_t<T>
}
class Charset_Utf8_helpers {
+constexpr bool is7bit(char c)
+constexpr bool is7bit(std::string_view sv)
}
class Array_deduction_guide {
<<deduction guide>>
+Array(T, U...) -> Array<T, 1 + sizeof...(U)>
}
Array_deduction_guide ..> Array_T_N
class OstreamDiffReporter {
+void printEnum(E x)
}
class MakeQPointer_constraints {
<<concept-style requires>>
}
MakeQPointer_constraints ..> MakeQPointer
class TinyRoomIdSet_convertTo {
+T convertTo(const U &input)
}
class Textures_typeHack {
+EnumIndexedArray typeHack(const T &input)
}
class RawExit_InvariantsHelper {
+void enforce() requires(not std::is_const_v<Exit_>)
}
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 2 issues, and left some high level feedback:
- The change to
numeric_hashfrom astd::enable_if_t<std::is_arithmetic_v<T>>template tonumeric_hash(const std::integral auto)drops support for floating-point types; if this was not intentional, consider either restoring arithmetic support or adding a separate overload for floats. - In
AnsiOstream::write, the newrequiresclause now explicitly excludeschar16_t(previously onlychar32_twas excluded); please double-check whether writingchar16_tvia this overload is still desired and, if so, adjust the constraint accordingly.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The change to `numeric_hash` from a `std::enable_if_t<std::is_arithmetic_v<T>>` template to `numeric_hash(const std::integral auto)` drops support for floating-point types; if this was not intentional, consider either restoring arithmetic support or adding a separate overload for floats.
- In `AnsiOstream::write`, the new `requires` clause now explicitly excludes `char16_t` (previously only `char32_t` was excluded); please double-check whether writing `char16_t` via this overload is still desired and, if so, adjust the constraint accordingly.
## Individual Comments
### Comment 1
<location path="src/global/MakeQPointer.h" line_range="15-16" />
<code_context>
template<typename T, typename... Args>
-NODISCARD auto makeQPointer(Args &&...args)
- -> std::enable_if_t<std::is_base_of_v<QObject, T>, QPointer<T>>
+ requires(std::is_base_of_v<QObject, T>)
+NODISCARD QPointer<T> makeQPointer(Args &&...args)
{
auto ptr = std::make_unique<T>(std::forward<Args>(args)...);
</code_context>
<issue_to_address>
**issue (bug_risk):** makeQPointer destroys the newly created QObject before returning, leaving QPointer dangling
This constructs a `std::unique_ptr<T>` and returns a `QPointer<T>` to `ptr.get()`. When `makeQPointer` returns, `ptr` is destroyed and `T` is deleted, so the `QPointer` immediately dangles. You need to transfer ownership out of the `unique_ptr` (e.g. `ptr.release()`) or construct with `new T(...)` so Qt owns the object via the parent hierarchy and it survives the function scope.
</issue_to_address>
### Comment 2
<location path="src/global/hash.h" line_range="11" />
<code_context>
-template<typename T>
-MAYBE_UNUSED NODISCARD static auto numeric_hash(const T val) noexcept
- -> std::enable_if_t<std::is_arithmetic_v<T>, size_t>
+MAYBE_UNUSED NODISCARD static size_t numeric_hash(const std::integral auto val) noexcept
{
- static constexpr const size_t size = sizeof(val);
</code_context>
<issue_to_address>
**question (bug_risk):** numeric_hash now only accepts integral types, which is a behavior change from the previous arithmetic version
Previously, `std::is_arithmetic_v<T>` allowed both integral and floating-point arguments; `const std::integral auto val` now rejects floats, so existing callers hashing `float`/`double` will break. If this narrowing is intentional, please document or rename accordingly; otherwise, consider a `requires std::is_arithmetic_v<T>` constraint or adding a `std::floating_point` overload to preserve prior behavior.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Comment on lines
+15
to
+16
| requires(std::is_base_of_v<QObject, T>) | ||
| NODISCARD QPointer<T> makeQPointer(Args &&...args) |
There was a problem hiding this comment.
issue (bug_risk): makeQPointer destroys the newly created QObject before returning, leaving QPointer dangling
This constructs a std::unique_ptr<T> and returns a QPointer<T> to ptr.get(). When makeQPointer returns, ptr is destroyed and T is deleted, so the QPointer immediately dangles. You need to transfer ownership out of the unique_ptr (e.g. ptr.release()) or construct with new T(...) so Qt owns the object via the parent hierarchy and it survives the function scope.
| template<typename T> | ||
| MAYBE_UNUSED NODISCARD static auto numeric_hash(const T val) noexcept | ||
| -> std::enable_if_t<std::is_arithmetic_v<T>, size_t> | ||
| MAYBE_UNUSED NODISCARD static size_t numeric_hash(const std::integral auto val) noexcept |
There was a problem hiding this comment.
question (bug_risk): numeric_hash now only accepts integral types, which is a behavior change from the previous arithmetic version
Previously, std::is_arithmetic_v<T> allowed both integral and floating-point arguments; const std::integral auto val now rejects floats, so existing callers hashing float/double will break. If this narrowing is intentional, please document or rename accordingly; otherwise, consider a requires std::is_arithmetic_v<T> constraint or adding a std::floating_point overload to preserve prior behavior.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #507 +/- ##
==========================================
- Coverage 25.10% 25.05% -0.05%
==========================================
Files 511 510 -1
Lines 42289 42275 -14
Branches 4574 4574
==========================================
- Hits 10617 10594 -23
- Misses 31672 31681 +9 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by Sourcery
Migrate the project to C++20 and adopt C++20 language/library features while tightening type constraints and constexpr usage.
Enhancements:
Build:
Documentation:
Tests: