[Lldb-commits] [PATCH] D56904: [NativePDB] Process virtual bases in the correct order
Zachary Turner via Phabricator via lldb-commits
lldb-commits at lists.llvm.org
Mon Jan 28 10:39:47 PST 2019
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
More information about the lldb-commits
mailing list