Skip to main content
Code Review

Return to Answer

added 490 characters in body
Source Link
JDługosz
  • 11.7k
  • 19
  • 40

template<typename OUT,typename IN> static inline constexpr OUT safe_numeric_cast( IN in ) {

It's hard to read with the template prefix and following declaration on one line like that. And again, it's quite odd to see inline static. Try:

template<typename OUT,typename IN>
constexpr OUT safe_numeric_cast ( IN in ) 
{

Furthermore, the use of IN and OUT may conflict with other libraries. I've seen those defined as macros that are used to mark parameters as with languages such as Ada or COM/CORBA marshalling.

The inline is not needed here to allow use inside a header. A template is not a function, just like how a cookie-cutter is not a cookie. There is no code there; and the instantiation of the template, which is a function, understands being instantiated in multiple translation units. Note that inline is no longer a hint used to ask to compiler to inline the function — the compiler decides for itself what to inline or not. It is only used to allow definitions to appear in headers so that multiple copies are merged. On the other hand, static means that the instantiated function will not be shared among different translation units, which is the exact opposite in meaning. And why would you want to do that?




template<typename OUT,typename IN> static inline constexpr OUT safe_numeric_cast( IN in ) {

It's hard to read with the template prefix and following declaration on one line like that. And again, it's quite odd to see inline static. Try:

template<typename OUT,typename IN>
constexpr OUT safe_numeric_cast ( IN in ) 
{

Furthermore, the use of IN and OUT may conflict with other libraries. I've seen those defined as macros that are used to mark parameters as with languages such as Ada or COM/CORBA marshalling.

The inline is not needed here to allow use inside a header. A template is not a function, just like how a cookie-cutter is not a cookie. There is no code there; and the instantiation of the template, which is a function, understands being instantiated in multiple translation units. Note that inline is no longer a hint used to ask to compiler to inline the function — the compiler decides for itself what to inline or not. It is only used to allow definitions to appear in headers so that multiple copies are merged. On the other hand, static means that the instantiated function will not be shared among different translation units, which is the exact opposite in meaning. And why would you want to do that?



added 490 characters in body
Source Link
JDługosz
  • 11.7k
  • 19
  • 40

I want most binary functions (operator+, etc...) to be compatible with two fixed objects, as well with one fixed object and one built-in arithmetic type - e.g. fixed + fixed, fixed + int, int + fixed. This currently leads to a lot of code duplication which I would like to get rid of.

In my implementation (which is Decimal, not power-of-two, and has different design requirements for many of your points), I decided to provide operator+ between identical types only. It would be confusing to have the result be a type that differs from either of the arguments and could lead to use of more distinct types than intended. On the other hand, operator+= does allow mixed types, as the result is implicitly the type of the left-hand operand, and it gives a compile-time error if the right-hand operand is larger in either size to the left of the decimal or size to the right of the decimal.

As for code duplication, I rely on operator= (only) to do the heavy lifting of implicitly widening the type. The addition and subtraction operators call that to condition the right-hand argument.

As for built-in integer type as one argument: are you doing size match checking? If you have, for example a 32-bit int with 5 implied fractional bits, then adding a regular 32-bit value will be too large to the left of the binary point. In my library, I disallow this, but constexpr constructors let me easily mark the regular int with a size that's smaller than the int. E.g. d1 += decimal<3,0>(n); tells it that n has (at most) three decimal digits (recall my library is decimal based) rather than 9 or 10 that would fit in a regular int. Thus, is passes the width checking and automatically widens to the declared type of d1 which is, say, 5 digits to the left and 4 to the right of the decimal point and represented in a 32-bit value.


Doing the underlying work as unsigned integers will be less efficient (less ability to optimize the inlined functions) as using signed values.


Why are your functions static inline instead of just inline? That is not normal.


You have namespaces nested as detail::fixed rather than the other way around? Also odd. Hmm, I see fixed is a class template that is not inside a namespace at all! You should put everything inside a namespace to prevent clashes.

I want most binary functions (operator+, etc...) to be compatible with two fixed objects, as well with one fixed object and one built-in arithmetic type - e.g. fixed + fixed, fixed + int, int + fixed. This currently leads to a lot of code duplication which I would like to get rid of.

In my implementation (which is Decimal, not power-of-two, and has different design requirements for many of your points), I decided to provide operator+ between identical types only. It would be confusing to have the result be a type that differs from either of the arguments and could lead to use of more distinct types than intended. On the other hand, operator+= does allow mixed types, as the result is implicitly the type of the left-hand operand, and it gives a compile-time error if the right-hand operand is larger in either size to the left of the decimal or size to the right of the decimal.

As for code duplication, I rely on operator= (only) to do the heavy lifting of implicitly widening the type. The addition and subtraction operators call that to condition the right-hand argument.

As for built-in integer type as one argument: are you doing size match checking? If you have, for example a 32-bit int with 5 implied fractional bits, then adding a regular 32-bit value will be too large to the left of the binary point. In my library, I disallow this, but constexpr constructors let me easily mark the regular int with a size that's smaller than the int. E.g. d1 += decimal<3,0>(n); tells it that n has (at most) three decimal digits (recall my library is decimal based) rather than 9 or 10 that would fit in a regular int. Thus, is passes the width checking and automatically widens to the declared type of d1 which is, say, 5 digits to the left and 4 to the right of the decimal point and represented in a 32-bit value.


I want most binary functions (operator+, etc...) to be compatible with two fixed objects, as well with one fixed object and one built-in arithmetic type - e.g. fixed + fixed, fixed + int, int + fixed. This currently leads to a lot of code duplication which I would like to get rid of.

In my implementation (which is Decimal, not power-of-two, and has different design requirements for many of your points), I decided to provide operator+ between identical types only. It would be confusing to have the result be a type that differs from either of the arguments and could lead to use of more distinct types than intended. On the other hand, operator+= does allow mixed types, as the result is implicitly the type of the left-hand operand, and it gives a compile-time error if the right-hand operand is larger in either size to the left of the decimal or size to the right of the decimal.

As for code duplication, I rely on operator= (only) to do the heavy lifting of implicitly widening the type. The addition and subtraction operators call that to condition the right-hand argument.

As for built-in integer type as one argument: are you doing size match checking? If you have, for example a 32-bit int with 5 implied fractional bits, then adding a regular 32-bit value will be too large to the left of the binary point. In my library, I disallow this, but constexpr constructors let me easily mark the regular int with a size that's smaller than the int. E.g. d1 += decimal<3,0>(n); tells it that n has (at most) three decimal digits (recall my library is decimal based) rather than 9 or 10 that would fit in a regular int. Thus, is passes the width checking and automatically widens to the declared type of d1 which is, say, 5 digits to the left and 4 to the right of the decimal point and represented in a 32-bit value.


Doing the underlying work as unsigned integers will be less efficient (less ability to optimize the inlined functions) as using signed values.


Why are your functions static inline instead of just inline? That is not normal.


You have namespaces nested as detail::fixed rather than the other way around? Also odd. Hmm, I see fixed is a class template that is not inside a namespace at all! You should put everything inside a namespace to prevent clashes.

Source Link
JDługosz
  • 11.7k
  • 19
  • 40

I want most binary functions (operator+, etc...) to be compatible with two fixed objects, as well with one fixed object and one built-in arithmetic type - e.g. fixed + fixed, fixed + int, int + fixed. This currently leads to a lot of code duplication which I would like to get rid of.

In my implementation (which is Decimal, not power-of-two, and has different design requirements for many of your points), I decided to provide operator+ between identical types only. It would be confusing to have the result be a type that differs from either of the arguments and could lead to use of more distinct types than intended. On the other hand, operator+= does allow mixed types, as the result is implicitly the type of the left-hand operand, and it gives a compile-time error if the right-hand operand is larger in either size to the left of the decimal or size to the right of the decimal.

As for code duplication, I rely on operator= (only) to do the heavy lifting of implicitly widening the type. The addition and subtraction operators call that to condition the right-hand argument.

As for built-in integer type as one argument: are you doing size match checking? If you have, for example a 32-bit int with 5 implied fractional bits, then adding a regular 32-bit value will be too large to the left of the binary point. In my library, I disallow this, but constexpr constructors let me easily mark the regular int with a size that's smaller than the int. E.g. d1 += decimal<3,0>(n); tells it that n has (at most) three decimal digits (recall my library is decimal based) rather than 9 or 10 that would fit in a regular int. Thus, is passes the width checking and automatically widens to the declared type of d1 which is, say, 5 digits to the left and 4 to the right of the decimal point and represented in a 32-bit value.


lang-cpp

AltStyle によって変換されたページ (->オリジナル) /