[Lldb-commits] [PATCH] D121967: [LLDB][NativePDB] Create inline function decls

[Reid Kleckner via Phabricator via lldb-commits]

rnk added inline comments.


================
Comment at: lldb/source/Plugins/SymbolFile/NativePDB/PdbAstBuilder.cpp:1123
+
+  PdbTypeSymId func_id(inline_site.Inlinee, true);
+  if (clang::Decl *decl = TryGetDecl(func_id))
----------------
Please add a comment about what the inlinee is, and what this lookup does. Basically, this lookup will find function declarations from previously inlined call sites.


================
Comment at: lldb/source/Plugins/SymbolFile/NativePDB/PdbAstBuilder.cpp:1153
+      StringIdRecord sir;
+      cantFail(
+          TypeDeserializer::deserializeAs<StringIdRecord>(parent_cvt, sir));
----------------
We shouldn't use cantFail if it isn't implied by local invariants. You should check if the parent scope is in fact a string first, otherwise this code will crash on invalid PDBs.


================
Comment at: lldb/source/Plugins/SymbolFile/NativePDB/PdbAstBuilder.cpp:1155
+          TypeDeserializer::deserializeAs<StringIdRecord>(parent_cvt, sir));
+      parent = GetOrCreateNamespaceDecl(sir.String.data(), *parent);
+    }
----------------
Can these be nested? You may need to split these on `::` and create  or look up nested namespace decls.


================
Comment at: lldb/source/Plugins/SymbolFile/NativePDB/PdbAstBuilder.cpp:1158
+
+    CVType func_type_cvt = m_index.tpi().getType(func_ti);
+    if (func_type_cvt.kind() == LF_PROCEDURE) {
----------------
This only requires `func_ti` as an input. Does it need to be part of the case? Should we compute param_count for methods too?


================
Comment at: lldb/source/Plugins/SymbolFile/NativePDB/PdbAstBuilder.cpp:1163
+          TypeDeserializer::deserializeAs<ProcedureRecord>(func_type_cvt, pr));
+      param_count = pr.getParameterCount();
+    }
----------------
This is overwritten later, so I think you can remove this if block.


================
Comment at: lldb/source/Plugins/SymbolFile/NativePDB/PdbAstBuilder.cpp:1174
+  func_ct = ToCompilerType(func_qt);
+  const clang::FunctionProtoType *func_type =
+      llvm::dyn_cast<clang::FunctionProtoType>(func_qt);
----------------
I don't know LLDB style, but LLVM style recommends using `auto` when casting so you do not need to repeat the casted type. This could be:
  const auto *func_type = llvm::dyn_cast<clang::FunctionProtoType>(func_qt);


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D121967