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

[Zachary Turner via Phabricator via lldb-commits]

zturner added inline comments.


================
Comment at: lldb/lit/SymbolFile/NativePDB/tag-types.cpp:87
+// Test single inheritance.
+class Derived : public Class {
+public:
----------------
lemo wrote:
> - at least one virtual function + override?
> - at least one method returning void?
The reason I didn't add any tests for methods is because I didn't add anything in the implementation to parse method records either.  As I said in the patch description, it was a balance between providing some piece of useful functionality while keeping the patch size down.  So I tried to limit the scope to just fields minus bitfields, since there's some extra complexity there that I wanted to punt on for now.  The reason I have the constructor is because it was required in order to initialize the reference member, otherwise it wouldn't compile.


================
Comment at: lldb/lit/SymbolFile/NativePDB/tag-types.cpp:110
+
+// Test multiple inheritance, as well as protected and private inheritance.
+class Derived2 : protected Class, private Struct {
----------------
lemo wrote:
> just an idea - add a virtual inheritance variation?
Virtual inheritance is a good followup, but if you look at `UdtRecordCompleter.cpp` and Ctrl+F for `VirtualBaseClassRecord`, I basically treat them as regular base classes and I put a fixme to handle the virtual aspects of the base.  So that's a known problem right now.


================
Comment at: lldb/lit/SymbolFile/NativePDB/tag-types.cpp:131
+
+int main(int argc, char **argv) {
+  Struct S;
----------------
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.


https://reviews.llvm.org/D53511