gnzlbg / static_vector

A dynamically-resizable vector with fixed capacity and embedded storage
https://gnzlbg.github.io/static_vector
167 stars 21 forks source link

Fix noninstantiated template copy/assignment functions #26

Closed mmha closed 7 years ago

mmha commented 7 years ago

The missing this-> pointers cause the compilation to fail with all versions of gcc. This happens because the argument matching happens after the name lookup, causing the compiler to unsuccessfully search after an overload of std::begin with zero arguments.

Also, std::enable_if_t has been incorrectly used in those functions.

Finally, I am not sure if you SFINAE with the correct condition. I think you meant Capacity < M and Capacity >= M instead of equality comparisons?

gnzlbg commented 7 years ago

Hey thank you a lot for the PR.

The missing this-> pointers cause the compilation to fail with all versions of gcc.

Yes this is a well known gcc bug that IIRC only affects constexpr functions (I would happily merge just this part).

Finally, I am not sure if you SFINAE with the correct condition. I think you meant Capacity < M and Capacity >= M instead of equality comparisons?

The implementation (which is just a prototype of an earlier version of the proposal) is out of sync with the proposal. I wouldn't recommend using it for anything serious. The proposal used to support interfacing fixed capacity vectors of different capacities, but it does not anymore.

I think the correct fix here would be to make the prototype match the current proposal, this has been on my TODO list for... way too long, but I have a complete implementation of this in a different project. If you want to tackle doing that I can point you to it (it would be to just basically port it to the std experimental namespace). Otherwise I might get to it this weekend and let you know.

mmha commented 7 years ago

Yes this is a well known gcc bug that IIRC only affects constexpr functions (I would happily merge just this part).

This is not a problem with constexpr. This minimal example does not compile either with all major compilers:

namespace N {
    void f(int){}  
}

void f() {}

int main() {
  using N::f;
  f();
}

on Compiler Explorer

If you want to tackle doing that I can point you to it

I would be happy to do so.

gnzlbg commented 7 years ago

@mmha I forgot to give you the link, it is here:

https://github.com/gnzlbg/hm3/blob/merge_old_fv/include/hm3/utility/fixed_capacity_vector.hpp#L65

gnzlbg commented 7 years ago

@mmha I've updated the implementation, let me know if it fixes this issue for you.