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

[Aleksandr Urakov via Phabricator via lldb-commits]

aleksandr.urakov added a comment.

I think that this patch is more solid and covers more cases, than https://reviews.llvm.org/D49368, especially in `CreateLLDBTypeFromPDBType` part. But https://reviews.llvm.org/D49368 has also following:

- Filling of a layout info. It allows use of packed types, bitfield structs with unnamed fields (unnamed fields themselves are not processed, but named fields are located correctly) etc., you can look at https://reviews.llvm.org/D49368 test case;
- Adding of method overrides for CXXRecordType.

In my opinion, it's not so difficult to add these features to this patch. What do you think about it?



================
Comment at: source/Plugins/SymbolFile/PDB/PDBASTParser.cpp:647-648
+        auto member_ast_type = type_sp->GetLayoutCompilerType();
+        if (!member_ast_type.IsCompleteType())
+          member_ast_type.GetCompleteType();
+        // CXX class type member must be parsed and complete ahead.
----------------
It's not so important, but I think that these lines can be deleted if arguments of subsequent `if` will be flipped.


================
Comment at: source/Plugins/SymbolFile/PDB/PDBASTParser.cpp:696
+          base_ast_type.GetOpaqueQualType(), access,
+          pdb_base_class->isVirtualBaseClass(), /*base_of_class*/ true);
+      base_classes.push_back(base_spec);
----------------
If I understand correctly, `base_of_class` must be `false` for structs. May be check the kind of `pdb_udt` here?


================
Comment at: source/Plugins/SymbolFile/PDB/SymbolFilePDB.cpp:587
+    // only do this once.
+    if (result->GetID() == type_uid) {
+      pdb->CompleteRecordTypeForPDBSymbol(*pdb_type, result);
----------------
I don't fully understand, please, explain me, when does this can be `false`?


================
Comment at: source/Plugins/SymbolFile/PDB/SymbolFilePDB.cpp:588
+    if (result->GetID() == type_uid) {
+      pdb->CompleteRecordTypeForPDBSymbol(*pdb_type, result);
+    }
----------------
What are the advantages and disadvantages of immediate (here) and deferred (at the `CompleteType` function) completions? I thought that deferred completion is a kind of lazy optimization, so we lost its advantages?


Repository:
  rL LLVM

https://reviews.llvm.org/D49410