New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
CppGenerator: operator<< take messages as const-ref #939
Conversation
Following commit 1973c86, using generated operator<< would require a mutable ref message. This is unnecessary and can be problematic. Ex: using wrappers such as fmt::streamed ( https://fmt.dev/latest/api.html#std-ostream-support )
Your patch breaks formatting messages with repeating groups, because access for reading repeating groups fields is non-const. Previous commit also could break exists code, because |
Hello @ksergey Apologies if I misunderstood your comment but method generateGroupDisplay was not modified. Dumping a Group should still require a mutable ref, should it not ? |
Hello @SBAM !
Yes, require a mutable ref Example ( ...
private:
FuelFigures m_fuelFigures;
public:
SBE_NODISCARD static SBE_CONSTEXPR std::uint16_t fuelFiguresId() SBE_NOEXCEPT
{
return 9;
}
SBE_NODISCARD inline FuelFigures &fuelFigures()
{
m_fuelFigures.wrapForDecode(m_buffer, sbePositionPtr(), m_actingVersion, m_bufferLength);
return m_fuelFigures;
}
FuelFigures &fuelFiguresCount(const std::uint16_t count)
{
m_fuelFigures.wrapForEncode(m_buffer, count, sbePositionPtr(), m_actingVersion, m_bufferLength);
return m_fuelFigures;
}
SBE_NODISCARD static SBE_CONSTEXPR std::uint64_t fuelFiguresSinceVersion() SBE_NOEXCEPT
{
return 0;
}
SBE_NODISCARD bool fuelFiguresInActingVersion() const SBE_NOEXCEPT
{
#if defined(__clang__)
#pragma clang diagnostic push
#pragma clang diagnostic ignored "-Wtautological-compare"
#endif
return m_actingVersion >= fuelFiguresSinceVersion();
#if defined(__clang__)
#pragma clang diagnostic pop
#endif
}
... |
Dumping directly a Group indeed mutates it since 1973c86. I assume that people who did that copied Group first. However, dumping a regular message copies initial data and passes to cascading groups dumpers that copy. template<typename CharT, typename Traits>
friend std::basic_ostream<CharT, Traits> & operator << (
std::basic_ostream<CharT, Traits> &builder, const MDIncrementalRefreshBook46 &_writer)
{
MDIncrementalRefreshBook46 writer(
_writer.m_buffer,
_writer.m_offset,
_writer.m_bufferLength,
_writer.m_actingBlockLength,
_writer.m_actingVersion);
builder << '{'; Are you suggesting to revert 1973c86 completely ? |
I think current implementation works pretty well |
I still can't quite grasp how adding that const will make current implementation work less well. |
@SBAM |
Dear all,
Following commit 1973c86, using generated operator<< would require a mutable ref message.
This is unnecessary and can be problematic.
For example when using wrappers such as fmt::streamed ( https://fmt.dev/latest/api.html#std-ostream-support ) that enforce const-correctness.