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



_______________________________________________
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

Reply via email to