[PATCH] D81319: [Dexter] Add --source-dir-root flag

Tobias Bosch via Phabricator via llvm-commits llvm-commits at lists.llvm.org
Thu Jun 11 11:34:52 PDT 2020


tbosch added inline comments.


================
Comment at: debuginfo-tests/dexter-tests/source-root-dir.cpp:1
+// REQUIRES: lldb
+// UNSUPPORTED: system-windows
----------------
TWeaver wrote:
> the dexter-tests folder is for debug-info tests that make use of dexter as a tool.
> 
> from the looks of it, this test is checking the new feature works?
> 
> if that's the case, this test should be in the dexter/feature-tests folder, in an appropriate place.
Ah, good point. Moved.


================
Comment at: debuginfo-tests/dexter/dex/debugger/DebuggerBase.py:183
+    def _external_to_debug_path(self, path):
+        root_dir = self.context.options.source_root_dir
+        if not root_dir or not path:
----------------
TWeaver wrote:
> One of our goals with dexter is to eradicate the over passing and over use of context throughout the code base.
> 
> There's a lot of work to do in this area and it may never happen but, one of the things we can do to ease the burden for any work we do in the future around this area is to do the following:
> 
> unpack anything you intend to use from context into a newly defined self.<thing_from_context> in the init method of the class that uses it.
> 
> use the new self.<thing_from_context> that you unpacked instead of self.context.<thing_from_context>.
> 
> This will hopefully reduce the burden in parsing long member look up chains as seen on this line.
> 
> It also means that, in future, when we look to remove context, we can know which bits of the context are required in the class without having to scout through the whole code base to find all the uses of it.
I am reading the options now in init. I can't read the option values yet as the debugger is used for creating options for the potential debuggers.


================
Comment at: debuginfo-tests/dexter/dex/debugger/DebuggerBase.py:187
+        assert path.startswith(root_dir)
+        return path[len(root_dir):].lstrip('/')
+
----------------
TWeaver wrote:
> what will be effect here on windows platforms? have you considered the case where windows paths have '\' instead?
I don't have a windows machine to test on :-(.
Changed this to use os.path.separator instead though.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D81319





More information about the llvm-commits mailing list