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

Zachary Turner via Phabricator via llvm-commits llvm-commits at lists.llvm.org
Tue Jul 17 09:42:21 PDT 2018


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





More information about the llvm-commits mailing list