[Lldb-commits] [PATCH] D126359: [LLDB] Add basic floating point ops to IR interpreter

[David Spickett via Phabricator via lldb-commits]

DavidSpickett added a reviewer: DavidSpickett.
DavidSpickett added a comment.

I know very little about the interpreter so someone else can comment there.

Certainly seems like a useful change. Added some generic comments.



================
Comment at: lldb/test/API/lang/c/fpeval/TestFPEval.py:1
+"""Show bitfields and check that they display correctly."""
+
----------------
Update this comment


================
Comment at: lldb/test/API/lang/c/fpeval/TestFPEval.py:21
+
+    def test_and_run_command(self):
+        """Test 'frame variable ...' on a variable with bitfields."""
----------------
I'm pretty sure anything starting with test will do, so I'd just call this `def test(self):`.


================
Comment at: lldb/test/API/lang/c/fpeval/TestFPEval.py:22
+    def test_and_run_command(self):
+        """Test 'frame variable ...' on a variable with bitfields."""
+        self.build()
----------------
Update this comment


================
Comment at: lldb/test/API/lang/c/fpeval/TestFPEval.py:51
+        self.expect("expr --allow-jit false  -- a == b", VARIABLES_DISPLAYED_CORRECTLY,
+                    substrs=['false'])
+
----------------
Probably a silly question but do you need `!=` here? I guess it becomes the inverse of `==` but wouldn't cost much to test it.


================
Comment at: lldb/test/API/lang/c/fpeval/main.c:3
+#include <stdio.h>
+#include <string.h>
+
----------------
I don't think you need any of these includes, certainly the latter 2.


================
Comment at: lldb/test/API/lang/c/fpeval/main.c:9
+    double b = 2.0;
+    return (long)(a + b); //// Set break point at this line.
+}
----------------
Small thing but could this line just be `return 0;` just saves a few cycles where you might think this line is being tested somehow when it really isn't.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D126359