[Lldb-commits] [PATCH] D53731: [NativePDB] Add the ability to display global variables

[Zachary Turner via Phabricator via lldb-commits]

zturner added inline comments.


================
Comment at: lldb/source/Plugins/SymbolFile/NativePDB/PdbSymUid.h:46
+                       // due to the debug magic at the beginning of the stream.
+  uint64_t global : 1; // True if this is from the globals stream.
+  uint64_t modi : 16;  // For non-global, this is the 0-based index of module.
----------------
lemo wrote:
> 30 + 1 != 32 - what's going on?
There's supposed to be a `uint64_t unused : 1;` here.  Thanks for noticing.


================
Comment at: lldb/source/Plugins/SymbolFile/NativePDB/SymbolFileNativePDB.cpp:913
+
+  uint32_t section_idx = section - 1;
+  if (section_idx >= section_list->GetSize())
----------------
lemo wrote:
> comment explaining the - 1 ?
I'm going to be totally honest here.  I don't understand this code at all.  I copied it from the `SymbolFilePDB` implementation.  It seems to work :-/


================
Comment at: lldb/source/Plugins/SymbolFile/NativePDB/SymbolFileNativePDB.cpp:936
+  lldb::ValueType scope = eValueTypeInvalid;
+  TypeIndex ti;
+  llvm::StringRef name;
----------------
lemo wrote:
> pls explicitly initialize all the locals
`TypeIndex` has a constructor, but I should definitely initialize the `lldb::addr_t`


================
Comment at: lldb/source/Plugins/SymbolFile/NativePDB/SymbolFileNativePDB.cpp:1406
+  TypeSP type_sp = CreateAndCacheType(uid);
+  return &*type_sp;
 }
----------------
lemo wrote:
> type_sp->get() seems cleaner / more readable
You know, I kept trying to write that, and I was like "what silly person on the C++ standards committee forgot to put a `get()` method on `std::shared_ptr<>`.  Did they really forget this or am I just going crazy?"  Turns out it's a bug in MSVC intellisense where it doesn't show it for some reason.  So I took that to mean it didn't exist.  Shame on me.


https://reviews.llvm.org/D53731