Skip to content

[APFloat] Fix APFloat::getOne #112308

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Oct 15, 2024
Merged

[APFloat] Fix APFloat::getOne #112308

merged 1 commit into from
Oct 15, 2024

Conversation

dtcxzyw
Copy link
Member

@dtcxzyw dtcxzyw commented Oct 15, 2024

APFloat::APFloat(const fltSemantics &Semantics, integerPart I) interprets 'I' as a unsigned integer.
Fix the bug found in #112113 (comment).

@llvmbot
Copy link
Member

llvmbot commented Oct 15, 2024

@llvm/pr-subscribers-llvm-adt

Author: Yingwei Zheng (dtcxzyw)

Changes

APFloat::APFloat(const fltSemantics &Semantics, integerPart I) interprets 'I' as a unsigned integer.
Fix the bug found in #112113 (comment).


Full diff: https://github.com/llvm/llvm-project/pull/112308.diff

2 Files Affected:

  • (modified) llvm/include/llvm/ADT/APFloat.h (+4-1)
  • (modified) llvm/unittests/ADT/APFloatTest.cpp (+7)
diff --git a/llvm/include/llvm/ADT/APFloat.h b/llvm/include/llvm/ADT/APFloat.h
index 6f1e24e5da33a9..97547fb577e0ec 100644
--- a/llvm/include/llvm/ADT/APFloat.h
+++ b/llvm/include/llvm/ADT/APFloat.h
@@ -1046,7 +1046,10 @@ class APFloat : public APFloatBase {
   ///
   /// \param Negative True iff the number should be negative.
   static APFloat getOne(const fltSemantics &Sem, bool Negative = false) {
-    return APFloat(Sem, Negative ? -1 : 1);
+    APFloat Val(Sem, 1U);
+    if (Negative)
+      Val.changeSign();
+    return Val;
   }
 
   /// Factory for Positive and Negative Infinity.
diff --git a/llvm/unittests/ADT/APFloatTest.cpp b/llvm/unittests/ADT/APFloatTest.cpp
index 6008f00e42a989..665cff9c424594 100644
--- a/llvm/unittests/ADT/APFloatTest.cpp
+++ b/llvm/unittests/ADT/APFloatTest.cpp
@@ -892,6 +892,13 @@ TEST(APFloatTest, Zero) {
   EXPECT_EQ(fcNegZero, APFloat(-0.0).classify());
 }
 
+TEST(APFloatTest, getOne) {
+  EXPECT_EQ(APFloat::getOne(APFloat::IEEEsingle(), false).convertToFloat(),
+            1.0f);
+  EXPECT_EQ(APFloat::getOne(APFloat::IEEEsingle(), true).convertToFloat(),
+            -1.0f);
+}
+
 TEST(APFloatTest, DecimalStringsWithoutNullTerminators) {
   // Make sure that we can parse strings without null terminators.
   // rdar://14323230.

Comment on lines 1048 to +1052
static APFloat getOne(const fltSemantics &Sem, bool Negative = false) {
return APFloat(Sem, Negative ? -1 : 1);
APFloat Val(Sem, 1U);
if (Negative)
Val.changeSign();
return Val;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We probably should generalize this in terms of a generate integer power of 2 helper

@dtcxzyw dtcxzyw merged commit a54d88f into llvm:main Oct 15, 2024
8 of 10 checks passed
@dtcxzyw dtcxzyw deleted the fix-apfloat-getone branch October 15, 2024 05:35
DanielCChen pushed a commit to DanielCChen/llvm-project that referenced this pull request Oct 16, 2024
`APFloat::APFloat(const fltSemantics &Semantics, integerPart I)`
interprets 'I' as a unsigned integer.
Fix the bug found in
llvm#112113 (comment).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants