[Lldb-commits] [PATCH] D82772: [lldb] Fix type conversion in the Scalar getters

[Pavel Labath via Phabricator via lldb-commits]

labath marked 3 inline comments as done.
labath added inline comments.


================
Comment at: lldb/source/Utility/Scalar.cpp:864
   case e_uint512:
-    return static_cast<long_double_t>(
-        llvm::APIntOps::RoundAPIntToDouble(m_integer));
----------------
teemperor wrote:
> Seems unrelated to the patch? Also it would be inconsistent that this is removed here and not below. FWIW this is used to suppress `-Wdouble-promotion` warnings, so it does have a purpose.
Yeah, I think this ended up being copied from the function above it.


================
Comment at: lldb/test/API/commands/expression/ir-interpreter/TestIRInterpreter.py:66
+                       "$i ^ $j",
+                       "($ull & -1) == $u"]
 
----------------
FTR, this was working correctly before already. I'm just creating a more explicit test for it.


================
Comment at: lldb/unittests/Utility/ScalarTest.cpp:97
+TEST(ScalarTest, Getters) {
+  CheckConversion((int)0x87654321);
+  CheckConversion((unsigned int)0x87654321u);
----------------
teemperor wrote:
> If you change this to `CheckConversion<int>(0x87654321);` then that's one C-style cast less (which will make me very happy) and if someone accidentally makes this a `long` literal or so we get a compiler warning (instead of the compiler just silently truncating the thing).
> 
> Also I guess `short` and `char` is missing?
Changed to `(T)` to `<T>`. `short` and `char` aren't interesting to test because Scalar does not have first-class support for them. They would get promoted to int before they reach the Scalar constructor. Long double is also missing -- that's because the relevant constructor is currently a booby-trap.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D82772/new/

https://reviews.llvm.org/D82772