[Lldb-commits] [PATCH] D145180: [lldb] Introduce new SymbolFileJSON and ObjectFileJSON

[Med Ismail Bennani via Phabricator via lldb-commits]

mib added a comment.

Left some comments but overall looks good to me :) Thanks for taking this !



================
Comment at: lldb/source/Plugins/ObjectFile/JSON/ObjectFileJSON.cpp:37
+                               offset_t file_offset, offset_t length) {
+  if (!data_sp) {
+    data_sp = MapFileData(*file, length, file_offset);
----------------



================
Comment at: lldb/source/Plugins/ObjectFile/JSON/ObjectFileJSON.cpp:47-52
+  if (data_sp->GetByteSize() < length) {
+    data_sp = MapFileData(*file, length, file_offset);
+    if (!data_sp)
+      return nullptr;
+    data_offset = 0;
+  }
----------------
Why do we do this twice ?


================
Comment at: lldb/source/Plugins/ObjectFile/JSON/ObjectFileJSON.cpp:92-111
+  if (!MagicBytesMatch(data_sp, data_offset, data_sp->GetByteSize()))
+    return 0;
+
+  auto text =
+      llvm::StringRef(reinterpret_cast<const char *>(data_sp->GetBytes()));
+
+  Expected<json::Value> json = json::parse(text);
----------------
I don't see the point of re-parsing the file here, we could just save the parsed object as a class member.


================
Comment at: lldb/source/Plugins/ObjectFile/JSON/ObjectFileJSON.cpp:145
+  uint32_t magic = data.GetU8(&offset);
+  return magic == '{';
+}
----------------
Cool 😁


================
Comment at: lldb/source/Plugins/SymbolFile/JSON/SymbolFileJSON.cpp:112
+    symtab.AddSymbol(Symbol(
+        /*symID*/ 0, Mangled(symbol.name), eSymbolTypeCode,
+        /*is_global*/ true, /*is_debug*/ false,
----------------
Should we increment the `symID` here ?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D145180