Skip to content
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

SFINAE fix to remove third argument in toXmlElementHelper() #490

Open
wants to merge 5 commits into
base: feature_state_serialization
Choose a base branch
from

Conversation

klshrinidhi
Copy link
Contributor

@klshrinidhi klshrinidhi commented May 7, 2016

This PR is to go into #488.

@chrisdembia
Copy link
Member

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

@klshrinidhi
Copy link
Contributor Author

I locally compiled and tested on gcc-5.

@sherm1
Copy link
Member

sherm1 commented May 7, 2016

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

@sherm1
Copy link
Member

sherm1 commented May 7, 2016

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

@klshrinidhi
Copy link
Contributor Author

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.

@chrisdembia
Copy link
Member

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.

@sherm1
Copy link
Member

sherm1 commented May 15, 2016

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.

@chrisdembia
Copy link
Member

chrisdembia commented May 15, 2016

@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
Copy link
Member

@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
Copy link
Member

@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
Copy link
Member

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.

@sherm1
Copy link
Member

sherm1 commented May 16, 2016

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

@chrisdembia
Copy link
Member

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.

@sherm1
Copy link
Member

sherm1 commented May 16, 2016

That could work. I doubt we need void_t though.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants