[Lldb-commits] [PATCH] D85705: Add a "Trace" plug-in to LLDB to add process trace support in stages.

[walter erquinigo via Phabricator via lldb-commits]

wallace marked 12 inline comments as done.
wallace added inline comments.


================
Comment at: lldb/source/Target/Trace.cpp:109
+  StringRef load_address_str;
+  if (!module->GetValueForKeyAsString("loadAddress", load_address_str))
+    return SetMissingFieldError(error, "loadAddress", "string", *module);
----------------
wallace wrote:
> clayborg wrote:
> > clayborg wrote:
> > > does JSON typically use camel case? Should this be "load-address"?
> > Should load address be encoded as an integer to begin with? I know it is less readable as an integer since we can't use hex numbers It would simplify the logic here. If we want to use strings, we should make a helper function that decodes an address from a key's value in a dictionary so we can re-use elsewhere.
> yes, it's camel case typically
The way this is implemented is that it's expecting a string number in any radix.  You can pass "123123" or "0xFFAABBFF" for example. You cannot pass it directly as a simple number because JSON doesn't support 64-bit integers


================
Comment at: lldb/source/Target/Trace.cpp:109-110
+  StringRef load_address_str;
+  if (!module->GetValueForKeyAsString("loadAddress", load_address_str))
+    return SetMissingFieldError(error, "loadAddress", "string", *module);
+  addr_t load_address;
----------------
clayborg wrote:
> clayborg wrote:
> > does JSON typically use camel case? Should this be "load-address"?
> Should load address be encoded as an integer to begin with? I know it is less readable as an integer since we can't use hex numbers It would simplify the logic here. If we want to use strings, we should make a helper function that decodes an address from a key's value in a dictionary so we can re-use elsewhere.
yes, it's camel case typically


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D85705