MikeLankamp / fpm

C++ header-only fixed-point math library
https://mikelankamp.github.io/fpm
MIT License
649 stars 79 forks source link

Add option to disable explicit conversions #55

Closed justanobdy closed 1 year ago

justanobdy commented 1 year ago

I thought it would be best to allow users to choose whether or not they want explicit conversions, so I added FPM_NO_EXPLICIT to disable explicit conversions. Explicit conversions are on by default, though.

MikeLankamp commented 1 year ago

It's an interesting idea, but I find it a dangerous one. Do you have a use case in mind where implicit (and potentially dangerous) conversions would be preferred over explicit ones?

justanobdy commented 1 year ago

I'm working on a library for the Nintendo DS which includes fpm (here) and conversions from a fixed-point variable to an integer are quite common, so it is often wanted to do these conversions, and being able to turn off explicit conversions makes it a little easier to write code (so that you don't have to explicitly cast every time), especially for beginners, and I try to make it as easy as possible for beginners with my library.

I agree that this is a dangerous one, and it might be fine if I just maintain my own fork which has this. Because it is useful in my library.

MikeLankamp commented 1 year ago

I believe safety of code and being able to trust code to be safe are big factors in writing good software. I think that allowing implicit conversions that may be unsafe are counter to that (i.e. similar to GCC's -Wconversion warning). I appreciate that you want to provide implicit conversion as an opt-in feature, but that (a) complicates the library and its testing (the feature would have to be tested, and I want to avoid having a variety of preprocessor feature macros that can end up influencing each other), and (b) goes against a core idea that this library was founded upon

In your case, I would recommend that if you find yourself disliking the verbosity of explicitly converting literals to fixed-point types, it might be worth using user-defined literals. For the non-literal cases, the explicit conversions serve as a warning that some information may be lost. And in my opinion this is something that should not be swept under the rug, even (or perhaps especially) for beginners.

So I unfortunately disagree with your intention behind this PR. I do want to thank you for raising the issue and being willing to contribute! I hope you find the alternatives above a better solution for your project than enabling implicit conversions in your fork of fpm.

justanobdy commented 1 year ago

Thank you for your detailed response, it's helpful. I'll research some more methods to see what I can do about this, and I agree with you that enabling implicit conversions in my fork isn't really the best idea.

I'm still learning programming, so I'm not always sure what the best programming practices are, so I'll take all this into account, thanks again.