[Lldb-commits] [PATCH] D56904: [NativePDB] Process virtual bases in the correct order

[Zachary Turner via Phabricator via lldb-commits]

zturner added a comment.

The above suggestion is admittedly minor, but since it's both a minor performance improvement **and** a minor readability/maintainability improvement, I think it's probably worth doing.



================
Comment at: lit/SymbolFile/NativePDB/tag-types.cpp:5
 // Test that we can display tag types.
 // RUN: %build --compiler=clang-cl --nodefaultlib -o %t.exe -- %s 
 // RUN: env LLDB_USE_NATIVE_PDB_READER=1 %lldb -f %t.exe -s \
----------------
aleksandr.urakov wrote:
> Clang gives me an error about char16_t, char32_t etc., but if I use MSVC here, then the test compiles ok.
Usually this means clang cannot find your CRT headers.  Can you run with -verbose?  That should give you some insight into the environment we are running clang with, and might be a clue why it can't find the headers.


================
Comment at: source/Plugins/SymbolFile/NativePDB/UdtRecordCompleter.cpp:54
+    llvm::codeview::TypeIndex ti, llvm::codeview::MemberAccess access,
+    bool is_virtual, uint64_t vtable_idx) {
   PdbTypeSymId type_id(ti);
----------------
Then, change this function to not pass `is_virtual`, only pass `Optional<vtable_idx>`.  


================
Comment at: source/Plugins/SymbolFile/NativePDB/UdtRecordCompleter.cpp:60-63
   std::unique_ptr<clang::CXXBaseSpecifier> base_spec =
       m_ast_builder.clang().CreateBaseClassSpecifier(
-          qt.getAsOpaquePtr(), TranslateMemberAccess(access), false,
+          qt.getAsOpaquePtr(), TranslateMemberAccess(access), is_virtual,
           udt_cvt.kind() == LF_CLASS);
----------------
`vtable_idx.hasValue();`


================
Comment at: source/Plugins/SymbolFile/NativePDB/UdtRecordCompleter.cpp:68-71
+  if (is_virtual)
+    m_vbases[vtable_idx] = std::move(base_spec);
+  else
+    m_bases.push_back(std::move(base_spec));
----------------
`m_bases.push_back(std::make_pair(vtable_idx.getValueOr(0), std::move(base_spec)));`


================
Comment at: source/Plugins/SymbolFile/NativePDB/UdtRecordCompleter.cpp:188
 void UdtRecordCompleter::complete() {
+  // Flush all virtual bases using the correct order.
+  for (auto &pair : m_vbases)
----------------
```
using vt = std::pair<uint64_t, std::unique_ptr<clang::CXXBaseSpecifier>>>;
std::stable_sort(m_bases, [](const vt &base1, const vt &base2) {
  return base1.first < base2.first;
  });
```


================
Comment at: source/Plugins/SymbolFile/NativePDB/UdtRecordCompleter.h:52
   std::vector<std::unique_ptr<clang::CXXBaseSpecifier>> m_bases;
+  std::map<uint64_t, std::unique_ptr<clang::CXXBaseSpecifier>> m_vbases;
   ClangASTImporter::LayoutInfo m_layout;
----------------
I actually think it would be slightly better to use a `vector<pair<>>` here.  The reason is that a) it will probably be slightly more efficient, because # of vbases is usually quite small (often just 1) and the overhead of a map is a net performance loss for small numbers of items.  b) that people are often confused when seeing `map` instead of `DenseMap` and it might cause someone in the future to think they can change this to a `DenseMap` with no behavioral difference, even though the reason for using a map here is that it must be sorted.

How about just having this:

```
std::vector<std::pair<uint64_t, std::unique_ptr<clang::CXXBaseSpecifier>>> m_bases;
```

Comments on how to change the implementation below.


Repository:
  rLLDB LLDB

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

https://reviews.llvm.org/D56904