[PATCH] D124511: [Dexter] Allow Dexter watch commands to specify a range of acceptable FP values

[Orlando Cazalet-Hyams via Phabricator via llvm-commits]

Orlando added a comment.

Just to confirm, does this comment:

> Remove unused "zero range" test.

mean that the feature I was worried about here:

> I'm not sure about this feature. What's the benefit of overriding 0.0 rather than just letting it mean zero?

doesn't exist, and the test was not correct for this iteration of the patch?

Please can you add a test for when the float range is set to `0.0` (easy / common edge case). LGTM with that plus inline comments addressed.



================
Comment at: cross-project-tests/debuginfo-tests/dexter/dex/command/commands/DexExpectWatchBase.py:183
 
+        expected_value = self._maybe_fix_float(step_info.expected_value)
+
----------------
StephenTozer wrote:
> Orlando wrote:
> > It's been a little while since I've looked in this code, am I reading this right - `expected_value` is the value returned by the debugger?
> Correct, I think the etymology is that it refers to the field of the watch value that we're expecting, i.e. the "value that we have an expectation for". (I do think it's confusing.)
Thanks for clarifying. It would be great if we're able to improve that name at some point in the future.


================
Comment at: cross-project-tests/debuginfo-tests/dexter/dex/command/commands/DexExpectWatchBase.py:156
+              difference = abs(value_as_float - expected_as_float)
+              if difference < self.float_range:
+                  return expected
----------------
Should this be `<=`? Otherwise an exact match with `float_range` of zero fails, right?


================
Comment at: cross-project-tests/debuginfo-tests/dexter/dex/utils/Exceptions.py:59
+    """IF a command instruction has the float_range arg but at least one of
+    it's expected values cannot be converted to a float."""
+
----------------
nit: IF -> if and it's -> its


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D124511