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
_______________________________________________
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
