lutraconsulting / MDAL

Mesh Data Abstraction Library
http://www.mdal.xyz/
MIT License
160 stars 50 forks source link

Question: c++11 required? #3

Closed nyalldawson closed 6 years ago

nyalldawson commented 6 years ago

I noticed in the source quite a lot of new/delete calls and factory methods which return raw pointers. Have you given any thought yet to a minimum version of c++ required to build this project?

If it's >= c++11, I'd suggest utilising unique_ptrs everywhere you can in the public api - so that e.g. factory methods always return a unique_ptr and ownership is never in doubt.

PeterPetrik commented 6 years ago

Hi @nyalldawson ... When I got a notification and read the title I was afraid that it is a follow-up to #1 and you want us to use plain C or better Assembler.

The only publicly visible header is https://github.com/lutraconsulting/MDAL/blob/master/mdal/api/mdal.h, which is C-only for more stable binary compatibility. We were thinking not to have C++ public interface at all.

But you are right that new/delete calls should be replaced with some c++11 smart pointers everywhere else.

nyalldawson commented 6 years ago

When I got a notification and read the title I was afraid that it is a follow-up to #1 and you want us to use plain C or better Assembler.

:laughing:

The exact opposite!

The only publicly visible header is https://github.com/lutraconsulting/MDAL/blob/master/mdal/api/mdal.h, which is C-only for more stable binary compatibility. We were thinking not to have C++ public interface at all.

Good move

But you are right that new/delete calls should be replaced with some c++11 smart pointers everywhere else.

I'm thinking also about the internal api - using unique_ptrs in interfaces will make the code much more robust and easy to hack on

PeterPetrik commented 6 years ago

Thanks a lot, going to look today on https://github.com/lutraconsulting/MDAL/pull/7

PeterPetrik commented 6 years ago

fixed by #7. Nice work!