simbody icon indicating copy to clipboard operation
simbody copied to clipboard

SFINAE fix to remove third argument in toXmlElementHelper()

Open klshrinidhi opened this issue 9 years ago • 14 comments

This PR is to go into #488.

klshrinidhi avatar May 07 '16 01:05 klshrinidhi

On travis, gcc gives a compiler error. @klshrinidhi says that later versions of gcc can compile this branch.

chrisdembia avatar May 07 '16 02:05 chrisdembia

I locally compiled and tested on gcc-5.

klshrinidhi avatar May 07 '16 02:05 klshrinidhi

Cool! But how does this prioritize the member function over the free function if both exist?

sherm1 avatar May 07 '16 05:05 sherm1

Oh, wait -- I'm just being slow. I get it. The free function isn't even there if the member function exists!

sherm1 avatar May 07 '16 05:05 sherm1

It did not compile with the gcc version you have. It is probably a feature they added in a later version. Up to you to decide what to do with this PR.

klshrinidhi avatar May 07 '16 06:05 klshrinidhi

I tested this PR with gcc 5.3.0 and it worked. It does not compile with gcc 4.9.3. I did not try any versions between these two.

chrisdembia avatar May 15 '16 04:05 chrisdembia

I'm sure there is a way to get the same idea to work in 4.9 with a little hacking around; I suspect it is just the void_t that is causing trouble. I'll try it as soon as I can get some time unless someone else gets to it first.

sherm1 avatar May 15 '16 17:05 sherm1

@sherm1 it seems you may be right. See stackoverflow, which says:

Here is a live example. It works fine with Clang, but unfortunately, GCC versions before 5.1 followed a different interpretation of the C++11 standard which caused void_t to not work as expected. Yakk already provided the work-around: use the following definition of void_t (void_t in parameter list works but not as return type):

#if __GNUC__ < 5 && ! defined __clang__
// http://stackoverflow.com/a/28967049/1353549
template <typename...>
struct voider
{
  using type = void;
};
template <typename...Ts>
using void_t = typename voider<Ts...>::type;
#else
template <typename...>
using void_t = void;
#endif

chrisdembia avatar May 15 '16 21:05 chrisdembia

@sherm1 @klshrinidhi I pushed the fix from stackoverflow. Thanks @sherm1 for the tip. I think we should probably move void_t to common.h, so it can be next to all the other macros that deal with different c++ standards.

chrisdembia avatar May 15 '16 21:05 chrisdembia

@sherm1 if you agree the void_t should go in common.h, let me know; I can make this change. Also, let me know if you think it should be in the SimTK namespace.

chrisdembia avatar May 15 '16 21:05 chrisdembia

Note: this fix is also mentioned here: http://en.cppreference.com/w/cpp/types/void_t:

template<typename... Ts> struct make_void { typedef void type;};
template<typename... Ts> using void_t = typename make_void<Ts...>::type;

I like make_void more than voider, so I think I'll change the name.

chrisdembia avatar May 15 '16 21:05 chrisdembia

I'm skeptical about adding void_t at all since it will be standardized in C++17 I think. Do we really need it?

sherm1 avatar May 16 '16 15:05 sherm1

I don't really understand the SFINAE stuff completely. My plan was to only define void_t ourselves if using a standard lower than 17.

chrisdembia avatar May 16 '16 16:05 chrisdembia

That could work. I doubt we need void_t though.

sherm1 avatar May 16 '16 17:05 sherm1