[Lldb-commits] [PATCH] D49410: [PDB] Parse UDT symbols and pointers to members

[Zachary Turner via Phabricator via lldb-commits]

zturner added inline comments.


================
Comment at: lit/SymbolFile/PDB/class-layout.test:3
+RUN: clang-cl -m32 /Z7 /c /GS- %S/Inputs/ClassLayoutTest.cpp /o %T/ClassLayoutTest.cpp.obj
+RUN: link %T/ClassLayoutTest.cpp.obj /DEBUG /nodefaultlib /ENTRY:main /OUT:%T/ClassLayoutTest.cpp.exe
+RUN: lldb-test symbols %T/ClassLayoutTest.cpp.exe | FileCheck %s
----------------
If you change this to `lld-link` this test may be able to run on non windows today.


================
Comment at: source/Plugins/SymbolFile/PDB/PDBASTParser.cpp:186
+    return lldb::eAccessProtected;
+  return lldb::eAccessNone;
+}
----------------
I wonder if this would be better as an `llvm_unreachable`.  Being able to assume this function returns a sane value would simplify calling code.


================
Comment at: source/Plugins/SymbolFile/PDB/PDBASTParser.cpp:198
+    return lldb::eAccessPrivate;
+  return lldb::eAccessNone;
+}
----------------
And here.


================
Comment at: source/Plugins/SymbolFile/PDB/PDBASTParser.cpp:246
+    return ty ? ty->shared_from_this() : nullptr;
+  } break;
   case PDB_SymType::UDT: {
----------------
This looks unusual.  Did you clang-format it?


================
Comment at: source/Plugins/SymbolFile/PDB/PDBASTParser.cpp:271-272
+    AccessType access = TranslateMemberAccess(udt->getAccess());
+    if (access == lldb::eAccessNone && udt->isNested() &&
+        udt->getClassParent()) {
+      access = GetDefaultAccessibilityForUdtKind(udt_kind);
----------------
So is this `access == None` a thing that can actually happen?  AFAICT it's impossible for `getAccess()` to return anything other than public, protected, or private, in which case this code path will never get executed, so we can just delete it.


================
Comment at: source/Plugins/SymbolFile/PDB/PDBASTParser.cpp:291-295
+    if (udt->isConstType())
+      clang_type = clang_type.AddConstModifier();
+
+    if (udt->isVolatileType())
+      clang_type = clang_type.AddVolatileModifier();
----------------
Can you remind me what this means?  How would an entire type be marked const or volatile?  There's no instance variable in play here, these are just types as they are declared in the source code, if I'm not mistaken.  Something like `const class Foo {int x; };` doesn't make any sense.  So I'm curious under what circumstances these 2 if statements would evaluate to true.


https://reviews.llvm.org/D49410