Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

std::initializer_list support is problematic and not needed #160

Closed
TieJu opened this issue Jan 21, 2014 · 3 comments
Closed

std::initializer_list support is problematic and not needed #160

TieJu opened this issue Jan 21, 2014 · 3 comments
Assignees
Labels
Milestone

Comments

@TieJu
Copy link

TieJu commented Jan 21, 2014

The explicit support for std::initializer_list is not really needed and creates some problems that can cause bugs.

The compiler try's to match the best matching constructor and the std::initializer_list one will grab all uniform initialization variants with any length and the compiler is no longer able to select the expected constructor.
This causes a list of problems and bugs:

  • any list that has not N elements (for vecN): undefined behavior ( if the list is shorter than N ) or assert fail.
  • copy/cast: the assignment at the constructor will fail because of type mismatch for each member.

You only need a std::initializer_list constructors for containers if your data type supports a construction of a list of variable number of elements, like std::vector, for data types with fixed length you gain nothing because the compiler already try's to match the other constructors to that list.

I tested this problematic behavior on ms vc++ 2013 ( and nov ctp ).

@Groovounet Groovounet added the bug label Feb 6, 2014
@Groovounet Groovounet added this to the GLM 0.9.5 milestone Feb 6, 2014
@Groovounet Groovounet self-assigned this Feb 6, 2014
@Groovounet Groovounet removed the bug label Feb 8, 2014
@Groovounet
Copy link
Member

Well in practice if I disable the initializer_list contructors and compile the core_type_mat4x4.cpp unit tests I get the following errors:

/Users/christophericcio/Documents/Repository/Github/glm/test/core/core_type_mat4x4.cpp:203:12:
No matching constructor for initialization of 'glm::mat4' (aka 'tmat4x4<float, highp>')
/Users/christophericcio/Documents/Repository/Github/glm/test/core/core_type_mat4x4.cpp:215:25: No matching constructor for initialization of 'std::vectorglm::mat4'

/Users/christophericcio/Documents/Repository/Github/glm/test/core/core_type_mat4x4.cpp:230:25: No matching constructor for initialization of 'std::vectorglm::mat4'

/Users/christophericcio/Documents/Repository/Github/glm/test/core/core_type_mat4x4.cpp:235:25: No matching constructor for initialization of 'std::vectorglm::mat4'

This was tested on Apple LLVM version 5.0 (clang-500.2.79) (based on LLVM 3.3svn).

I also tested the behaviour of VC13 and it's very similar.

Thanks,
Christophe

@TieJu
Copy link
Author

TieJu commented Feb 8, 2014

You need to remove the 'explicit'ness of many constructors that are needed to do the initialization of mat4 and vec4, then it will work.
Explicit is only needed for one parameter constructors to prohibit transformations from one type to an other, like from a single int to a vec or matrix, what is clearly not desired, for multi-param constructors this makes the constructors unusable for such desired transformations.

@Groovounet
Copy link
Member

This issue is fixed in GLM 0.9.5 branch for next.

I removed the initializer_list constructors and removed some explicit constructors.

Thanks for contributing,
Christophe

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

No branches or pull requests

2 participants