TextStream operator<<: add size_t overload#100
Conversation
📝 WalkthroughWalkthrough
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@framework/global/serialization/textstream.cpp`:
- Around line 79-81: The static_assert(false, ...) in the else branch is
evaluated during parsing in uninstantiated templates, causing premature failures
in C++17/20. Create a dependent-false helper variable template called
always_false_v at namespace scope before the function containing this code, then
replace the static_assert(false, ...) with static_assert(always_false_v<I>, ...)
to defer evaluation until template instantiation, making the assertion dependent
on the template parameter I.
In `@framework/global/serialization/textstream.h`:
- Around line 55-58: Remove the redundant operator<<(size_t) overload from the
TextStream class declaration. Since size_t and uint64_t are identical types on
all target platforms, the operator<<(uint64_t) overload already provides the
necessary functionality for handling size_t values. Deleting the
operator<<(size_t) declaration will resolve the One Definition Rule violation
and eliminate the compilation error.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: e8c9484b-aad3-4913-98e4-cd3d9e7fed3a
📒 Files selected for processing (2)
framework/global/serialization/textstream.cppframework/global/serialization/textstream.h
| } else { | ||
| static_assert(false, "Unsupported integral type size"); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n framework/global/serialization/textstream.cpp | head -100Repository: musescore/muse_framework
Length of output: 3218
🏁 Script executed:
rg "static_assert\(false" --type cppRepository: musescore/muse_framework
Length of output: 178
🏁 Script executed:
# Verify the proposed solution syntax and context
sed -n '68,82p' framework/global/serialization/textstream.cppRepository: musescore/muse_framework
Length of output: 563
🏁 Script executed:
# Check if the proposed patch would compile
# First, let's test if we can use the helper inside the function context
cat > /tmp/test_static_assert.cpp << 'EOF'
`#include` <type_traits>
template<typename I>
constexpr size_t max_digits()
{
if constexpr (sizeof(I) == 4) {
return 10;
} else if constexpr (sizeof(I) == 8) {
return 20;
} else {
template<typename>
inline constexpr bool always_false_v = false;
static_assert(always_false_v<I>, "Unsupported integral type size");
}
}
int main() {
return max_digits<int>();
}
EOF
# Check if standard C++ compilation fails (we don't have a compiler, so just analyze)
echo "Note: Checking the proposed syntax..."
cat /tmp/test_static_assert.cppRepository: musescore/muse_framework
Length of output: 524
🏁 Script executed:
# Search for any existing dependent-false patterns in the codebase
rg "always_false|dependent.*false" --type cppRepository: musescore/muse_framework
Length of output: 183
🌐 Web query:
C++ static_assert false template dependent false helper pattern
💡 Result:
In C++ templates, a static_assert(false, "message") declaration is evaluated during the initial parsing of the template definition [1][2]. Because the expression is not dependent on any template parameters, the compiler can determine that it will always be false and will issue an error, even if the template is never instantiated [1][3][4]. To defer the assertion until the template is actually instantiated, you must make the condition type-dependent [1][5]. This prevents the compiler from evaluating the expression during the initial parsing phase [1][4]. The standard idiom for this is to use a helper variable template that is always false but depends on the template parameter(s) [6][5][7]: template <typename... T> inline constexpr bool always_false_v = false; // Usage in a template: template struct MyClass { static_assert(always_false_v, "This template must be specialized"); }; By using always_false_v, the expression becomes dependent on T. The compiler must wait until an actual instantiation occurs (e.g., MyClass) to evaluate it, at which point it correctly triggers the static assertion failure [1][3]. C++23 Update: As of C++23 (via CWG 2518 / P2593R1), static_assert(false,...) is now well-formed in uninstantiated templates [7][8]. The language was updated to delay the evaluation of all static assertions in templates until the point of instantiation, effectively rendering the "dependent false" helper pattern unnecessary for this specific purpose in modern C++ codebases [9][7].
Citations:
- 1: https://devblogs.microsoft.com/oldnewthing/20200311-00/?p=103553
- 2: https://learn.microsoft.com/en-us/cpp/cpp/static-assert?view=msvc-170
- 3: https://stackoverflow.com/questions/76673888/why-is-always-false-v-required-in-this-situation
- 4: https://stackoverflow.com/questions/26726321/does-static-assert-require-use-of-template-parameter
- 5: https://artificial-mind.net/blog/2020/10/03/always-false
- 6: https://www.open-std.org/jtc1/sc22/wg21/docs/papers/2019/p1830r1.pdf
- 7: https://wwww.cppreference.com/w/cpp/language/static_assert.html
- 8: https://cppreference.dev/w/cpp/language/static_assert
- 9: https://open-std.org/jtc1/sc22/wg21/docs/papers/2023/p2593r1.html
Use a dependent-false helper to make the template static_assert dependent
static_assert(false, ...) in an uninstantiated template is evaluated during parsing in C++17/20, causing premature failures. Use a dependent-false helper variable to defer evaluation until instantiation.
Suggested approach
Define a helper at namespace scope before the function:
namespace {
template<typename>
inline constexpr bool always_false_v = false;
}Then in the else branch:
static_assert(always_false_v<I>, "Unsupported integral type size");Note: The variable template must be defined at namespace scope, not inside the function body.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@framework/global/serialization/textstream.cpp` around lines 79 - 81, The
static_assert(false, ...) in the else branch is evaluated during parsing in
uninstantiated templates, causing premature failures in C++17/20. Create a
dependent-false helper variable template called always_false_v at namespace
scope before the function containing this code, then replace the
static_assert(false, ...) with static_assert(always_false_v<I>, ...) to defer
evaluation until template instantiation, making the assertion dependent on the
template parameter I.
|
Would it not make more sense to remove all the integer-typed overrides and implement the template function directly in the header? Now that we are on C++20 we could use a concept to limit it to integer types only. (I believe SFNAE would take care of the Something like: template<typename T>
concept NonCharInteger =
std::integral<T> &&
!std::same_as<std::remove_cv_t<T>, char> &&
!std::same_as<std::remove_cv_t<T>, signed char> &&
!std::same_as<std::remove_cv_t<T>, unsigned char> &&
!std::same_as<std::remove_cv_t<T>, wchar_t> &&
!std::same_as<std::remove_cv_t<T>, char8_t> &&
!std::same_as<std::remove_cv_t<T>, char16_t> &&
!std::same_as<std::remove_cv_t<T>, char32_t>;
template<NonCharInteger I>
TextStream& TextStream::operator<<(I v)
{
// ...
return *this;
}The current template definition does not really solve the problem of eliminating the possibility of some future unexpected int type causing an error. Plus, I don't think it will compile on systems where |
|
The problem is that you soon end up having to put the whole implementation in the header file, because otherwise you get linker errors. That was my reason for not making the entire function a template, and for keeping the explicit overloads. But it looks like making it a template is unavoidable, so we'll have to either accept putting the implementation in the header, or we'd have to deal with the linking problems, I think by making sure the templates are instantiated in the same translation unit where they are defined. |
|
Yes, it is possible to instantiate known implementations in the TU. Doing it that way may not even care if there are declarations for the same type (uint64_t and size_t). |
|
Another way to avoid putting the implementation in the header and also avoid instantiating explicit templates would be to defer to one of two non-template implementation function that accepts the int value static casted to signed or unsigned 64-bit ints along with a max size allowed. |
cbjeukendrup
force-pushed
the
textstream-size_t
branch
from
1cfbc39 to
146202d
Compare
|
Pushed another attempt. |
|
You may know more than I, but I thought it had to be like this: template<NonCharInteger I>
TextStream& TextStream::operator<<(I v) |
|
What I wrote was what my auto-complete came up with, but it is a shorthand notation for what you wrote. Anyway, apparently this repo doesn't do C++20 yet, so we'll have to resort to enable_if. Sigh. |
|
I think it is getting there. Why not |
|
I think that would run into the instantiation problem, but I'll try |
size_t != uint64_t on macOS, despite being a 64-bit unsigned integer. Make it a template, so that all integer types work. However after that, it broke for std::int8_t, aka signed char, so added an explicit overload for that.
Surely not if the size calc is in the header. |
cbjeukendrup
force-pushed
the
textstream-size_t
branch
from
146202d to
58dd93d
Compare
|
Oh, never mind. You are correct. It could at least just be 64 bit implementations, with the size passed as a parameter. That would get rid of two of the wrappers (maybe: you're closer to it than I am.) |
2 participants
Necessary to solve a compilation issue in MuseScore’s tests on some platforms (including macOS)