| Message ID | 20260918120949.191668-2-barnabas.pocze@ideasonboard.com |
|---|---|
| State | New |
| Headers | show |
| Series |
|
| Related | show |
Quoting Barnabás Pőcze (2026-09-18 13:09:29) > Avoid shifting into the potential sign bit of `T`. The same is already > done in `toFloat()`. > > Signed-off-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com> > --- > src/ipa/libipa/fixedpoint.h | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/src/ipa/libipa/fixedpoint.h b/src/ipa/libipa/fixedpoint.h > index ed8aca9f15..537bc774a4 100644 > --- a/src/ipa/libipa/fixedpoint.h > +++ b/src/ipa/libipa/fixedpoint.h > @@ -78,7 +78,7 @@ public: > * properly cast negative values. See > * https://embeddeduse.com/2013/08/25/casting-a-negative-float-to-an-unsigned-int/ > */ > - return static_cast<UT>(static_cast<T>(std::round(v * (1 << F)))) & bitMask; > + return static_cast<UT>(static_cast<T>(std::round(v * (UT{ 1 } << F)))) & bitMask; Ayee, brings back so many painful memories ;-) Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > } > }; > > -- > 2.55.0 >
Hi Barnabás On Fri, Sep 18, 2026 at 02:09:29PM +0200, Barnabás Pőcze wrote: > Avoid shifting into the potential sign bit of `T`. The same is already > done in `toFloat()`. > > Signed-off-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com> > --- > src/ipa/libipa/fixedpoint.h | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/src/ipa/libipa/fixedpoint.h b/src/ipa/libipa/fixedpoint.h > index ed8aca9f15..537bc774a4 100644 > --- a/src/ipa/libipa/fixedpoint.h > +++ b/src/ipa/libipa/fixedpoint.h > @@ -78,7 +78,7 @@ public: > * properly cast negative values. See > * https://embeddeduse.com/2013/08/25/casting-a-negative-float-to-an-unsigned-int/ This seems to be broken ? > */ > - return static_cast<UT>(static_cast<T>(std::round(v * (1 << F)))) & bitMask; > + return static_cast<UT>(static_cast<T>(std::round(v * (UT{ 1 } << F)))) & bitMask; If the issue you're trying to prevent is int a = 1 << 31; ==> -2147483648 shouldn't the result of the shift be casted to UT rather than just the 1 ? Because int a = 1U << 31; still gives me a negative number. IOW isn't the destination type of the shift that decided if the number should be interpreted as negative or positive ? > } > }; > > -- > 2.55.0 >
2026. 09. 23. 16:31 keltezéssel, Jacopo Mondi írta: > Hi Barnabás > > On Fri, Sep 18, 2026 at 02:09:29PM +0200, Barnabás Pőcze wrote: >> Avoid shifting into the potential sign bit of `T`. The same is already >> done in `toFloat()`. >> >> Signed-off-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com> >> --- >> src/ipa/libipa/fixedpoint.h | 2 +- >> 1 file changed, 1 insertion(+), 1 deletion(-) >> >> diff --git a/src/ipa/libipa/fixedpoint.h b/src/ipa/libipa/fixedpoint.h >> index ed8aca9f15..537bc774a4 100644 >> --- a/src/ipa/libipa/fixedpoint.h >> +++ b/src/ipa/libipa/fixedpoint.h >> @@ -78,7 +78,7 @@ public: >> * properly cast negative values. See >> * https://embeddeduse.com/2013/08/25/casting-a-negative-float-to-an-unsigned-int/ > > This seems to be broken ? > >> */ >> - return static_cast<UT>(static_cast<T>(std::round(v * (1 << F)))) & bitMask; >> + return static_cast<UT>(static_cast<T>(std::round(v * (UT{ 1 } << F)))) & bitMask; > > If the issue you're trying to prevent is > > int a = 1 << 31; ==> -2147483648 > > shouldn't the result of the shift be casted to UT rather than just the > 1 ? > > Because > int a = 1U << 31; > > still gives me a negative number. > > IOW isn't the destination type of the shift that decided if the number > should be interpreted as negative or positive ? In the `v * ...` multiplication, the result of the shift gets converted to a floating point type, so there is only a UT -> float conversion, not UT -> T. > > > >> } >> }; >> >> -- >> 2.55.0 >>
Hi Barnabás On Wed, Sep 23, 2026 at 04:37:23PM +0200, Barnabás Pőcze wrote: > 2026. 09. 23. 16:31 keltezéssel, Jacopo Mondi írta: > > Hi Barnabás > > > > On Fri, Sep 18, 2026 at 02:09:29PM +0200, Barnabás Pőcze wrote: > > > Avoid shifting into the potential sign bit of `T`. The same is already > > > done in `toFloat()`. > > > > > > Signed-off-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com> > > > --- > > > src/ipa/libipa/fixedpoint.h | 2 +- > > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > > > diff --git a/src/ipa/libipa/fixedpoint.h b/src/ipa/libipa/fixedpoint.h > > > index ed8aca9f15..537bc774a4 100644 > > > --- a/src/ipa/libipa/fixedpoint.h > > > +++ b/src/ipa/libipa/fixedpoint.h > > > @@ -78,7 +78,7 @@ public: > > > * properly cast negative values. See > > > * https://embeddeduse.com/2013/08/25/casting-a-negative-float-to-an-unsigned-int/ > > > > This seems to be broken ? > > > > > */ > > > - return static_cast<UT>(static_cast<T>(std::round(v * (1 << F)))) & bitMask; > > > + return static_cast<UT>(static_cast<T>(std::round(v * (UT{ 1 } << F)))) & bitMask; > > > > If the issue you're trying to prevent is > > > > int a = 1 << 31; ==> -2147483648 > > > > shouldn't the result of the shift be casted to UT rather than just the > > 1 ? > > > > Because > > int a = 1U << 31; > > > > still gives me a negative number. > > > > IOW isn't the destination type of the shift that decided if the number > > should be interpreted as negative or positive ? > > In the `v * ...` multiplication, the result of the shift gets converted to a > floating point type, so there is only a UT -> float conversion, not UT -> T. ack. Would using 1U instead of UT{ 1 } make any difference, iow would it save constructing an object ? A detail anyway Reviewed-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com> Thanks j > > > > > > > > > > > } > > > }; > > > > > > -- > > > 2.55.0 > > > >
diff --git a/src/ipa/libipa/fixedpoint.h b/src/ipa/libipa/fixedpoint.h index ed8aca9f15..537bc774a4 100644 --- a/src/ipa/libipa/fixedpoint.h +++ b/src/ipa/libipa/fixedpoint.h @@ -78,7 +78,7 @@ public: * properly cast negative values. See * https://embeddeduse.com/2013/08/25/casting-a-negative-float-to-an-unsigned-int/ */ - return static_cast<UT>(static_cast<T>(std::round(v * (1 << F)))) & bitMask; + return static_cast<UT>(static_cast<T>(std::round(v * (UT{ 1 } << F)))) & bitMask; } };
Avoid shifting into the potential sign bit of `T`. The same is already done in `toFloat()`. Signed-off-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com> --- src/ipa/libipa/fixedpoint.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)