[Lldb-commits] [PATCH] D148172: [lldb] Use ObjectFileJSON to create modules for interactive crashlogs

[Jonas Devlieghere via Phabricator via lldb-commits]

JDevlieghere marked 7 inline comments as done.
JDevlieghere added inline comments.


================
Comment at: lldb/examples/python/crashlog.py:460-464
+    def __init__(self, debugger, path, verbose):
+        super().__init__(debugger, path, verbose)
+        # List of DarwinImages sorted by their index.
+        self.images = list()
+
----------------
mib wrote:
> Why do we need this ?
The parser now stores a list of images sorted by their index. We can't use the list of images in the crashlog (`self.crashlog.images`) because they have the main module at index `0`.


================
Comment at: lldb/examples/python/crashlog.py:515
     def parse_images(self, json_images):
-        idx = 0
-        for json_image in json_images:
+        for idx, json_image in enumerate(json_images):
             img_uuid = uuid.UUID(json_image['uuid'])
----------------
mib wrote:
> What do we use `idx` for ?
You're right, this isn't necessary anymore.


================
Comment at: lldb/examples/python/crashlog.py:526-527
                                                      self.verbose)
+            self.images.append(darwin_image)
             self.crashlog.images.append(darwin_image)
 
----------------
mib wrote:
> Seems like we're doing the same things for both `self.images` and `self.crashlog.images` ... In that case, is `self.images` really necessary ?
Yes, see my previous comment. 


================
Comment at: lldb/examples/python/symbolication.py:402-403
+                            })
+                    for symbol in self.symbols.values():
+                        data['symbols'].append(symbol)
+                    with open(tf.name, 'w') as f:
----------------
mib wrote:
> This should work, right ?
Apparently not: `values()` returns a view object rather than a list: https://docs.python.org/3/library/stdtypes.html#dict-views


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

https://reviews.llvm.org/D148172