[Lldb-commits] [PATCH] D148062: [lldb] Make ObjectFileJSON loadable as a module

[Med Ismail Bennani via Phabricator via lldb-commits]

mib added inline comments.


================
Comment at: lldb/lldb/test/API/macosx/symbols/TestObjectFileJSON.py:12
+
+class TestObjectFileJSOn(TestBase):
+    TRIPLE = "arm64-apple-macosx13.0.0"
----------------
nit


================
Comment at: lldb/source/Core/Section.cpp:683-684
+  llvm::json::ObjectMapper o(value, path);
+  return o && o.map("name", section.name) && o.map("type", section.type) &&
+         o.map("size", section.address) && o.map("size", section.size);
+}
----------------
We should use `o.mapOptional` here since the `address`, `size` and `type` are marked optional.


================
Comment at: lldb/source/Plugins/ObjectFile/JSON/ObjectFileJSON.cpp:111
 
+  if (data_sp->GetByteSize() < length) {
+    data_sp = MapFileData(file, length, file_offset);
----------------
I guess this is due to the fact that we only read the first 512 bytes from the binary the first time we load it ? It would be good to mention it in a comment.


================
Comment at: lldb/source/Plugins/ObjectFile/JSON/ObjectFileJSON.cpp:210
+  return o && o.map("triple", header.triple) && o.map("uuid", header.uuid) &&
+         o.map("type", header.type);
 }
----------------
ditto


================
Comment at: lldb/test/API/macosx/symbols/TestObjectFileJSON.py:12
+
+class TestObjectFileJSOn(TestBase):
+    TRIPLE = "arm64-apple-macosx13.0.0"
----------------
It looks like you've included this test twice.


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

https://reviews.llvm.org/D148062