Hello Michael, Thanks for taking the time to help me understand that. So I've reworked the PR here https://github.com/openvehicles/Open-Vehicle-Monitoring-System-3/pull/736 with your implementation (and added you as a co-author if you are OK) which seems to work as expected (I changed a few things to make it work). If you (and/or anybody else who is using DBC...) can review and approve this final version, we would then be ready for a merge. Thanks again, Le 22/10/2022 à 15:49, Michael Geddes a écrit :
The other benefit of templating rather than macros is that macros will insert the code as it is in the parameter, potentially causing some unnecessary execution of code, or unwanted side effects, where as with a template function, you still get the benefit of code expansion, but the parameters are evaluated and then used in the inline expanded function.
I could possibly have made this a little better.. but it wouldn't have changed the code generation much. Ahh.. so in my use case, the sign-bin is templated (fixed at compile time), which would mean that all that shifting arithmetic gets done at compile-time... but in your case, we would need to have it as a parameter. /** Sign extend an unsigned to a signed integer of the same size. */ template<typename UINT, typename INT> INT sign_extend( UINT uvalue, uint8_t signbit) { typedef typename std::make_unsigned<INT>::type uint_t; uint_t newuvalue = uvalue; if ( newuvalue & ( UINT(1U) << signbit)) ) { newuvalue |= ~((uint_t(1U) << signbit)) - 1); } return reinterpret_cast<INT &>(uvalue); }
It would be possible to have both define and use the one that suits! then the code would become:
if (m_value_type == DBC_VALUETYPE_UNSIGNED) result.Cast((uint32_t)val, DBC_NUMBER_INTEGER_UNSIGNED); else { int32_t iVal = sign_extend<uint32_t, int32_t>((uint32_t)val,m_signal_size-1); result.Cast(static_cast<uint32_t>(iVal), DBC_NUMBER_INTEGER_SIGNED); }
in the sign_extend parameters, it is designed to be able to have the unsigned and signed types as different sizes. For example sign_extend<uint8_t, int64_t>(val, signbit); This would sign-extend the 8bit unsigned input into 64 bit signed.
YOu can use your old code.. but be aware that (m_signal_size-1) would be copied out ,and evaluated twice! Rather than evaluated once.
//.ichael
On Sat, 22 Oct 2022 at 21:08, Ludovic LANGE <ll-ovmsdev@lange.nom.fr> wrote:
Hi Michael,
Thank you a lot for your message... however I must admit my co//.ichaemplete ignorance (and lack of practice) of the whole C++ / templating world. I mean that I cannot understand (yet) your template version - such a shame ; nor adapt it to this fix.
Just if you have a few minutes to enlighten me : if we were to look at the proposed patch here https://github.com/openvehicles/Open-Vehicle-Monitoring-System-3/pull/736/fi... ; and if we would introduce your template implementation, how should we rewrite the "patched" version to make use of this implementation ?
result.Cast((uint32_t)SIGNEX(val, m_signal_size-1), DBC_NUMBER_INTEGER_SIGNED);
(my naïve attempts all failed compilation with all sorts of "error: use of 'this' in a constant expression" and "error: no matching function for call to 'sign_extend(uint64_t&)'" so a little help would be appreciated)
Additionally, could you please expand a little on the benefits it adds - you're talking about type-safety, if you could just illustrate it in this context ?
Sorry for being ignorant on this subject.
Regards,
Ludovic