mhier
commented
2 years ago We should use int32_t, uint64_t etc. always when specifying the UserType template argument of the DeviceAccess accessors and in related places. The use of int is at least sloppy, the use of long and size_t etc. is certainly wrong, for the exact reasons you are quoting.
The very long error message probably also points to some missing static_assert somewhere (potentially DeviceAccess? Maybe can you send me the error message somehow to confirm?), which could give a clearer error message if wrong types are used...
So in short, yes I agree.
smarsching
When compiling the control-system adapter with Clang 12.0 (on macOS 12.6, x86_64) there is a problem when compiling
testUnifiedTypeChangingDecorator.cpp. The error message is very, very long, so I am not reproducing it here, but the problem seems to boil down to the fact that inBOOST_AUTO_TEST_CASE(testRegisterAccessor), we instantiate sub-classes ofTestRegisterspecifying the typelongas the template parameter.When replacing
longwithint32_torint64_t, the code compiles without any issues, so I think that this is caused by the fact that inSupportedUserTypes.h(from DeviceAccess),int32_tandint64_tare listed butlongis not. Interestingly,intis listed neither, but can be used.I suspect that Clang (at least the version of Clang provided for macOS 12), does not assume that
long == int64_tfor the purpose of overload / template resolution. I think that this might be related to the problem withsize_tanduint64_tthat we saw in DeviceAccess (see https://github.com/ChimeraTK/DeviceAccess/issues/281).I didn’t find anything about this in the Clang documentation, but I found a discussion about the same problem from a few years ago: https://comp.lang.cpp.narkive.com/kH1S9MDr/int32-t-vs-int64-t-vs-long-ambiguous
The behavior of Clang actually makes sense, because the nasty thing about
longis that it might be 32 or 64 bit depending on the platform (see https://en.cppreference.com/w/cpp/language/types), so overload / template resolution would depend on the platform, which could result in some very hard to find problems.In general, mixing the fixed-size integer types with the platform-dependent types is a bad idea, so I think we should replace
intandlongintestUnifiedTypeChangingDecorator.cppwithint32_tandint64_trespectively. Usingintis not quite as problematic aslongbecausesizeof(int) == 4seems to be true on most platforms (Win16 being one notable exception, but I don’t think that Clang even supports this platform), but using this in this context still is not entriely clean, so I would like to avoid it.Are there any objections?