search

jk-jeon / fp

IEEE-754 binary-to-decimal and decimal-to-binary conversion library
40 stars 5 forks source link

Unnecessary Check #7

Closed sirinath closed 3 years ago

sirinath commented 3 years ago

Checking here is not needed. There is no instance where shift_amount of 0 is passed. Besides the result is the same.

sirinath commented 3 years ago

Some of the compile time code perhaps can be optimised further. For this implementation there is not runtime impact but when porting to other languages it might be helpful when there is no constexpr.

jk-jeon commented 3 years ago

Checking here is not needed. There is no instance where shift_amount of 0 is passed. Besides the result is the same.

The function floor_shift is only called inside a constexpr context, so as you have mentioned it does not impact the runtime performance. And I doubt it will really have a measurable impact on the compiler-time performance as well. There are too many similar floor_log type functions and it was not fun to manage all of them independently, thus I tried to merge them with some compile-time abstraction. And I think the most important is the correctness and robustness when it comes to compile-time programming. I agree that it is unlikely that the case shift_amount == 0 will be ever useful, but I believe there is little to loose here by being a bit more careful.

when porting to other languages it might be helpful when there is no constexpr.

Could you elaborate on this?

sirinath commented 3 years ago

I am trying to port this to Java. All the I am converting all the constexpr to static which in this case has a runtime penalty.

jk-jeon commented 3 years ago

I think in that case you should write it differently.

sirinath commented 3 years ago

Yes.

But also some of the code can be written with translation in mind to help porters to other languages.

jk-jeon commented 3 years ago

That's fair enough. I'll keep that in mind from now on.