[PATCH] D107070: [Dexter] Improve Dexter's performance by evaluating expressions only when needed

[Jeremy Morse via Phabricator via llvm-commits]

jmorse added a comment.

LGTM in principle, although I agree with Chris, the get_watches block is a bit too pythonic, and contains a generator within a generator. It'd be best to split that into statements, IMO.

It's not ideal that the debugger wrappers have to be adjusted to benefit from these improvements; I guess that's a future refactoring project though.



================
Comment at: cross-project-tests/debuginfo-tests/dexter/dex/command/commands/DexExpectWatchBase.py:153
     def eval(self, step_collection):
+        assert os.path.exists(self.path)
         for step in step_collection.steps:
----------------
I have mixed feelings about this; IMO we want to steer away from relying on the source files being accessible by Dexter while running. But on the other hand, the previous implementation on this function relied on it for correctness, all this assert is doing is drawing attention to the fact that if the file isn't there, you won't get the results you wanted.

Overall I would call this an improvement, as it accelerates our likelyhood of fixing this.


================
Comment at: cross-project-tests/debuginfo-tests/dexter/dex/debugger/DebuggerBase.py:21-22
 
+def watch_is_active(watch_info, path, frame_idx, line_no):
+    _, watch_path, watch_frame_idx, watch_line_range = watch_info
+    if (watch_path and os.path.isfile(watch_path) and
----------------
Adding a type annotation, docu-comment or otherwise to indicate the type of watch_info would be good. It'll be hard for returning readers otherwise.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D107070