[Lldb-commits] [PATCH] D53511: [NativePDB] Support type lookup by name

[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 to me, minor notes inline.



================
Comment at: lldb/lit/SymbolFile/NativePDB/tag-types.cpp:131
+
+int main(int argc, char **argv) {
+  Struct S;
----------------
zturner wrote:
> lemo wrote:
> > a few suggestions for additional things to cover:
> > - local classes
> > - lambdas
> > - instantiating class and function templates
> >    - explicit specializations
> >    - for classes, partial specializations
> > - namespaces
> > - anonymous namespace
> Some of these I could probably handle right now.  I tried to keep the scope of the patch to "types which would be named", because it makes it easy to write tests (the test can just do lookup by name, basically a WinDbg `dt` test.).  That makes local classes, lambdas, anonymous namespace, and anything to do with functions out of scope.
k. how about more basic stuff like?
- pointer to pointer
- const/volatile



================
Comment at: lldb/source/Plugins/SymbolFile/NativePDB/SymbolFileNativePDB.cpp:313
+  case SimpleTypeKind::SByte:
+    return "signed chr";
+  case SimpleTypeKind::Character16:
----------------
char


================
Comment at: lldb/source/Plugins/SymbolFile/NativePDB/SymbolFileNativePDB.cpp:335
+  case SimpleTypeKind::Int64Quad:
+    return "__int64";
+  case SimpleTypeKind::Int32:
----------------
int64_t ?


================
Comment at: lldb/source/Plugins/SymbolFile/NativePDB/SymbolFileNativePDB.cpp:344
+  case SimpleTypeKind::UInt64Quad:
+    return "unsigned __int64";
+  case SimpleTypeKind::HResult:
----------------
unsigned int64_t ?


================
Comment at: lldb/source/Plugins/SymbolFile/NativePDB/SymbolFileNativePDB.cpp:660
+    ct = ct.GetPointerType();
+    uint32_t pointer_size = 4;
+    switch (ti.getSimpleMode()) {
----------------
minor: = 0; ? 
(to make any potential mistakes more obvious)


================
Comment at: lldb/source/Plugins/SymbolFile/NativePDB/SymbolFileNativePDB.cpp:687
+  CompilerType ct = m_clang->GetBasicType(bt);
+  size_t size = GetTypeSizeForSimpleKind(ti.getSimpleKind());
+
----------------
assert size > 0 ?


================
Comment at: lldb/source/Plugins/SymbolFile/NativePDB/UdtRecordCompleter.cpp:189
+}
\ No newline at end of file

----------------
nit: add empty line at the end of file



================
Comment at: lldb/source/Plugins/SymbolFile/NativePDB/UdtRecordCompleter.h:26
+namespace lldb_private {
+class Type;
+class CompilerType;
----------------
nit: vertical space between { and the next line


https://reviews.llvm.org/D53511