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

[Leonard Mosescu via Phabricator via lldb-commits]

lemo accepted this revision.
lemo added a comment.
This revision is now accepted and ready to land.

looks good. a few comments inline.



================
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.
----------------
30 + 1 != 32 - what's going on?


================
Comment at: lldb/source/Plugins/SymbolFile/NativePDB/PdbSymUid.h:135
+  static PdbSymUid makeGlobalVariableUid(uint32_t offset) {
+    PdbSymUid uid;
+    uid.m_uid.cu_sym.modi = 0;
----------------
 = {}; ? (to avoid uninitialized bits)


================
Comment at: lldb/source/Plugins/SymbolFile/NativePDB/SymbolFileNativePDB.cpp:892
 
+static DWARFExpression MakeGlobalLocationExpression(uint16_t section,
+                                                    uint32_t offset,
----------------
assert that section > 0 ? (as precondition)


================
Comment at: lldb/source/Plugins/SymbolFile/NativePDB/SymbolFileNativePDB.cpp:913
+
+  uint32_t section_idx = section - 1;
+  if (section_idx >= section_list->GetSize())
----------------
comment explaining the - 1 ?


================
Comment at: lldb/source/Plugins/SymbolFile/NativePDB/SymbolFileNativePDB.cpp:936
+  lldb::ValueType scope = eValueTypeInvalid;
+  TypeIndex ti;
+  llvm::StringRef name;
----------------
pls explicitly initialize all the locals


================
Comment at: lldb/source/Plugins/SymbolFile/NativePDB/SymbolFileNativePDB.cpp:1406
+  TypeSP type_sp = CreateAndCacheType(uid);
+  return &*type_sp;
 }
----------------
type_sp->get() seems cleaner / more readable


https://reviews.llvm.org/D53731