[Lldb-commits] [PATCH] D156498: [lldb] Support recursive record types in CTF

[Michael Buch via Phabricator via lldb-commits]

Michael137 added a comment.

Generally LGTM, just some clarifications questions



================
Comment at: lldb/source/Plugins/SymbolFile/CTF/SymbolFileCTF.cpp:508
                               ctf_record.name.data(), tag_kind, eLanguageTypeC);
+  m_compiler_types[record_type.GetOpaqueQualType()] = &ctf_record;
+  Declaration decl;
----------------
Just to clarify the lifetimes. This `ctf_record` lives on `m_ast` while the `m_compiler_types` on the SymbolFile, so we're guaranteed that the `ctf_record` lives long enough?


================
Comment at: lldb/source/Plugins/SymbolFile/CTF/SymbolFileCTF.cpp:524-529
+  // We only support resolving record types.
+  assert(ctf_type->kind == CTFType::Kind::eStruct ||
+         ctf_type->kind == CTFType::Kind::eUnion);
 
-  m_ast->StartTagDeclarationDefinition(record_type);
-  for (const CTFRecord::Field &field : ctf_record.fields) {
-    if (Type *field_type = ResolveTypeUID(field.type)) {
-      const uint32_t field_size = field_type->GetByteSize(nullptr).value_or(0);
-      TypeSystemClang::AddFieldToRecordType(record_type, field.name,
-                                            field_type->GetFullCompilerType(),
-                                            eAccessPublic, field_size);
+  // Cast to the appropriate CTF type.
+  const CTFRecord *ctf_record = static_cast<const CTFRecord *>(ctf_type);
----------------
Nit: would it make sense to just add `classof` methods to `CTFType` and use the llvm cast facilities?

Feel free to ignore since there's just one instance of such cast afaict


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D156498/new/

https://reviews.llvm.org/D156498