[Lldb-commits] [lldb] r345312 - [NFC] Refactor SetBaseClasses and DeleteBaseClasses.
Zachary Turner via lldb-commits
lldb-commits at lists.llvm.org
Thu Oct 25 13:44:56 PDT 2018
Author: zturner
Date: Thu Oct 25 13:44:56 2018
New Revision: 345312
URL: http://llvm.org/viewvc/llvm-project?rev=345312&view=rev
Log:
[NFC] Refactor SetBaseClasses and DeleteBaseClasses.
We currently had a 2-step process where we had to call
SetBaseClassesForType and DeleteBaseClasses. Every single caller
followed this exact 2-step process, and there was manual memory
management going on with raw pointers. We can do better than this
by storing a vector of unique_ptrs and passing this around.
This makes for a cleaner API, and we only need to call one method
so there is no possibility of a user forgetting to call
DeleteBaseClassSpecifiers.
In addition to this, it also makes for a *simpler* API. Part of
why I wanted to do this is because when I was implementing the native
PDB interface I had to spend some time understanding exactly what I
was deleting and why. ClangAST has significant mental overhead
associated with it, and reducing the API surface can go along
way to making it simpler for people to understand.
Differential Revision: https://reviews.llvm.org/D53590
Modified:
lldb/trunk/include/lldb/Symbol/ClangASTContext.h
lldb/trunk/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp
lldb/trunk/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.h
lldb/trunk/source/Plugins/SymbolFile/NativePDB/UdtRecordCompleter.cpp
lldb/trunk/source/Plugins/SymbolFile/NativePDB/UdtRecordCompleter.h
lldb/trunk/source/Plugins/SymbolFile/PDB/PDBASTParser.cpp
lldb/trunk/source/Symbol/ClangASTContext.cpp
lldb/trunk/unittests/Symbol/TestClangASTContext.cpp
Modified: lldb/trunk/include/lldb/Symbol/ClangASTContext.h
URL: http://llvm.org/viewvc/llvm-project/lldb/trunk/include/lldb/Symbol/ClangASTContext.h?rev=345312&r1=345311&r2=345312&view=diff
==============================================================================
--- lldb/trunk/include/lldb/Symbol/ClangASTContext.h (original)
+++ lldb/trunk/include/lldb/Symbol/ClangASTContext.h Thu Oct 25 13:44:56 2018
@@ -858,18 +858,14 @@ public:
void AddMethodOverridesForCXXRecordType(lldb::opaque_compiler_type_t type);
// C++ Base Classes
- clang::CXXBaseSpecifier *
+ std::unique_ptr<clang::CXXBaseSpecifier>
CreateBaseClassSpecifier(lldb::opaque_compiler_type_t type,
lldb::AccessType access, bool is_virtual,
bool base_of_class);
- static void DeleteBaseClassSpecifiers(clang::CXXBaseSpecifier **base_classes,
- unsigned num_base_classes);
-
- bool
- SetBaseClassesForClassType(lldb::opaque_compiler_type_t type,
- clang::CXXBaseSpecifier const *const *base_classes,
- unsigned num_base_classes);
+ bool TransferBaseClasses(
+ lldb::opaque_compiler_type_t type,
+ std::vector<std::unique_ptr<clang::CXXBaseSpecifier>> bases);
static bool SetObjCSuperClass(const CompilerType &type,
const CompilerType &superclass_compiler_type);
Modified: lldb/trunk/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp
URL: http://llvm.org/viewvc/llvm-project/lldb/trunk/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp?rev=345312&r1=345311&r2=345312&view=diff
==============================================================================
--- lldb/trunk/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp (original)
+++ lldb/trunk/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp Thu Oct 25 13:44:56 2018
@@ -2184,14 +2184,14 @@ bool DWARFASTParserClang::CompleteTypeFr
}
SymbolContext sc(die.GetLLDBCompileUnit());
- std::vector<clang::CXXBaseSpecifier *> base_classes;
+ std::vector<std::unique_ptr<clang::CXXBaseSpecifier>> bases;
std::vector<int> member_accessibilities;
bool is_a_class = false;
// Parse members and base classes first
DWARFDIECollection member_function_dies;
DelayedPropertyList delayed_properties;
- ParseChildMembers(sc, die, clang_type, class_language, base_classes,
+ ParseChildMembers(sc, die, clang_type, class_language, bases,
member_accessibilities, member_function_dies,
delayed_properties, default_accessibility, is_a_class,
layout_info);
@@ -2255,11 +2255,11 @@ bool DWARFASTParserClang::CompleteTypeFr
&member_accessibilities.front(), member_accessibilities.size());
}
- if (!base_classes.empty()) {
+ if (!bases.empty()) {
// Make sure all base classes refer to complete types and not forward
// declarations. If we don't do this, clang will crash with an
- // assertion in the call to clang_type.SetBaseClassesForClassType()
- for (auto &base_class : base_classes) {
+ // assertion in the call to clang_type.TransferBaseClasses()
+ for (const auto &base_class : bases) {
clang::TypeSourceInfo *type_source_info =
base_class->getTypeSourceInfo();
if (type_source_info) {
@@ -2278,7 +2278,7 @@ bool DWARFASTParserClang::CompleteTypeFr
// We have no choice other than to pretend that the base class
// is complete. If we don't do this, clang will crash when we
// call setBases() inside of
- // "clang_type.SetBaseClassesForClassType()" below. Since we
+ // "clang_type.TransferBaseClasses()" below. Since we
// provide layout assistance, all ivars in this class and other
// classes will be fine, this is the best we can do short of
// crashing.
@@ -2290,14 +2290,9 @@ bool DWARFASTParserClang::CompleteTypeFr
}
}
}
- m_ast.SetBaseClassesForClassType(clang_type.GetOpaqueQualType(),
- &base_classes.front(),
- base_classes.size());
-
- // Clang will copy each CXXBaseSpecifier in "base_classes" so we have
- // to free them all.
- ClangASTContext::DeleteBaseClassSpecifiers(&base_classes.front(),
- base_classes.size());
+
+ m_ast.TransferBaseClasses(clang_type.GetOpaqueQualType(),
+ std::move(bases));
}
}
}
@@ -2675,7 +2670,7 @@ Function *DWARFASTParserClang::ParseFunc
bool DWARFASTParserClang::ParseChildMembers(
const SymbolContext &sc, const DWARFDIE &parent_die,
CompilerType &class_clang_type, const LanguageType class_language,
- std::vector<clang::CXXBaseSpecifier *> &base_classes,
+ std::vector<std::unique_ptr<clang::CXXBaseSpecifier>> &base_classes,
std::vector<int> &member_accessibilities,
DWARFDIECollection &member_function_dies,
DelayedPropertyList &delayed_properties, AccessType &default_accessibility,
@@ -3271,9 +3266,14 @@ bool DWARFASTParserClang::ParseChildMemb
if (class_language == eLanguageTypeObjC) {
ast->SetObjCSuperClass(class_clang_type, base_class_clang_type);
} else {
- base_classes.push_back(ast->CreateBaseClassSpecifier(
- base_class_clang_type.GetOpaqueQualType(), accessibility,
- is_virtual, is_base_of_class));
+ std::unique_ptr<clang::CXXBaseSpecifier> result =
+ ast->CreateBaseClassSpecifier(
+ base_class_clang_type.GetOpaqueQualType(), accessibility,
+ is_virtual, is_base_of_class);
+ if (!result)
+ break;
+
+ base_classes.push_back(std::move(result));
if (is_virtual) {
// Do not specify any offset for virtual inheritance. The DWARF
Modified: lldb/trunk/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.h
URL: http://llvm.org/viewvc/llvm-project/lldb/trunk/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.h?rev=345312&r1=345311&r2=345312&view=diff
==============================================================================
--- lldb/trunk/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.h (original)
+++ lldb/trunk/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.h Thu Oct 25 13:44:56 2018
@@ -80,16 +80,16 @@ protected:
lldb_private::ClangASTContext::TemplateParameterInfos
&template_param_infos);
- bool
- ParseChildMembers(const lldb_private::SymbolContext &sc, const DWARFDIE &die,
- lldb_private::CompilerType &class_compiler_type,
- const lldb::LanguageType class_language,
- std::vector<clang::CXXBaseSpecifier *> &base_classes,
- std::vector<int> &member_accessibilities,
- DWARFDIECollection &member_function_dies,
- DelayedPropertyList &delayed_properties,
- lldb::AccessType &default_accessibility, bool &is_a_class,
- lldb_private::ClangASTImporter::LayoutInfo &layout_info);
+ bool ParseChildMembers(
+ const lldb_private::SymbolContext &sc, const DWARFDIE &die,
+ lldb_private::CompilerType &class_compiler_type,
+ const lldb::LanguageType class_language,
+ std::vector<std::unique_ptr<clang::CXXBaseSpecifier>> &base_classes,
+ std::vector<int> &member_accessibilities,
+ DWARFDIECollection &member_function_dies,
+ DelayedPropertyList &delayed_properties,
+ lldb::AccessType &default_accessibility, bool &is_a_class,
+ lldb_private::ClangASTImporter::LayoutInfo &layout_info);
size_t
ParseChildParameters(const lldb_private::SymbolContext &sc,
Modified: lldb/trunk/source/Plugins/SymbolFile/NativePDB/UdtRecordCompleter.cpp
URL: http://llvm.org/viewvc/llvm-project/lldb/trunk/source/Plugins/SymbolFile/NativePDB/UdtRecordCompleter.cpp?rev=345312&r1=345311&r2=345312&view=diff
==============================================================================
--- lldb/trunk/source/Plugins/SymbolFile/NativePDB/UdtRecordCompleter.cpp (original)
+++ lldb/trunk/source/Plugins/SymbolFile/NativePDB/UdtRecordCompleter.cpp Thu Oct 25 13:44:56 2018
@@ -59,12 +59,12 @@ lldb::opaque_compiler_type_t UdtRecordCo
CVType udt_cvt = m_symbol_file.m_index->tpi().getType(ti);
lldb::opaque_compiler_type_t base_qt = base_ct.GetOpaqueQualType();
- clang::CXXBaseSpecifier *base_spec =
+ std::unique_ptr<clang::CXXBaseSpecifier> base_spec =
m_symbol_file.GetASTContext().CreateBaseClassSpecifier(
base_qt, TranslateMemberAccess(access), false,
udt_cvt.kind() == LF_CLASS);
-
- m_bases.push_back(base_spec);
+ lldbassert(base_spec);
+ m_bases.push_back(std::move(base_spec));
return base_qt;
}
@@ -172,9 +172,8 @@ Error UdtRecordCompleter::visitKnownMemb
void UdtRecordCompleter::complete() {
ClangASTContext &clang = m_symbol_file.GetASTContext();
- clang.SetBaseClassesForClassType(m_derived_ct.GetOpaqueQualType(),
- m_bases.data(), m_bases.size());
- ClangASTContext::DeleteBaseClassSpecifiers(m_bases.data(), m_bases.size());
+ clang.TransferBaseClasses(m_derived_ct.GetOpaqueQualType(),
+ std::move(m_bases));
clang.AddMethodOverridesForCXXRecordType(m_derived_ct.GetOpaqueQualType());
ClangASTContext::BuildIndirectFields(m_derived_ct);
Modified: lldb/trunk/source/Plugins/SymbolFile/NativePDB/UdtRecordCompleter.h
URL: http://llvm.org/viewvc/llvm-project/lldb/trunk/source/Plugins/SymbolFile/NativePDB/UdtRecordCompleter.h?rev=345312&r1=345311&r2=345312&view=diff
==============================================================================
--- lldb/trunk/source/Plugins/SymbolFile/NativePDB/UdtRecordCompleter.h (original)
+++ lldb/trunk/source/Plugins/SymbolFile/NativePDB/UdtRecordCompleter.h Thu Oct 25 13:44:56 2018
@@ -40,7 +40,7 @@ class UdtRecordCompleter : public llvm::
CompilerType &m_derived_ct;
clang::TagDecl &m_tag_decl;
SymbolFileNativePDB &m_symbol_file;
- std::vector<clang::CXXBaseSpecifier *> m_bases;
+ std::vector<std::unique_ptr<clang::CXXBaseSpecifier>> m_bases;
ClangASTImporter::LayoutInfo m_layout;
public:
Modified: lldb/trunk/source/Plugins/SymbolFile/PDB/PDBASTParser.cpp
URL: http://llvm.org/viewvc/llvm-project/lldb/trunk/source/Plugins/SymbolFile/PDB/PDBASTParser.cpp?rev=345312&r1=345311&r2=345312&view=diff
==============================================================================
--- lldb/trunk/source/Plugins/SymbolFile/PDB/PDBASTParser.cpp (original)
+++ lldb/trunk/source/Plugins/SymbolFile/PDB/PDBASTParser.cpp Thu Oct 25 13:44:56 2018
@@ -1208,7 +1208,8 @@ void PDBASTParser::AddRecordBases(
lldb_private::CompilerType &record_type, int record_kind,
PDBBaseClassSymbolEnumerator &bases_enum,
lldb_private::ClangASTImporter::LayoutInfo &layout_info) const {
- std::vector<clang::CXXBaseSpecifier *> base_classes;
+ std::vector<std::unique_ptr<clang::CXXBaseSpecifier>> base_classes;
+
while (auto base = bases_enum.getNext()) {
auto base_type = symbol_file.ResolveTypeUID(base->getTypeId());
if (!base_type)
@@ -1229,13 +1230,13 @@ void PDBASTParser::AddRecordBases(
auto is_virtual = base->isVirtualBaseClass();
- auto base_class_spec = m_ast.CreateBaseClassSpecifier(
- base_comp_type.GetOpaqueQualType(), access, is_virtual,
- record_kind == clang::TTK_Class);
- if (!base_class_spec)
- continue;
+ std::unique_ptr<clang::CXXBaseSpecifier> base_spec =
+ m_ast.CreateBaseClassSpecifier(base_comp_type.GetOpaqueQualType(),
+ access, is_virtual,
+ record_kind == clang::TTK_Class);
+ lldbassert(base_spec);
- base_classes.push_back(base_class_spec);
+ base_classes.push_back(std::move(base_spec));
if (is_virtual)
continue;
@@ -1247,13 +1248,9 @@ void PDBASTParser::AddRecordBases(
auto offset = clang::CharUnits::fromQuantity(base->getOffset());
layout_info.base_offsets.insert(std::make_pair(decl, offset));
}
- if (!base_classes.empty()) {
- m_ast.SetBaseClassesForClassType(record_type.GetOpaqueQualType(),
- &base_classes.front(),
- base_classes.size());
- ClangASTContext::DeleteBaseClassSpecifiers(&base_classes.front(),
- base_classes.size());
- }
+
+ m_ast.TransferBaseClasses(record_type.GetOpaqueQualType(),
+ std::move(base_classes));
}
void PDBASTParser::AddRecordMethods(lldb_private::SymbolFile &symbol_file,
Modified: lldb/trunk/source/Symbol/ClangASTContext.cpp
URL: http://llvm.org/viewvc/llvm-project/lldb/trunk/source/Symbol/ClangASTContext.cpp?rev=345312&r1=345311&r2=345312&view=diff
==============================================================================
--- lldb/trunk/source/Symbol/ClangASTContext.cpp (original)
+++ lldb/trunk/source/Symbol/ClangASTContext.cpp Thu Oct 25 13:44:56 2018
@@ -8239,39 +8239,37 @@ void ClangASTContext::AddMethodOverrides
#pragma mark C++ Base Classes
-clang::CXXBaseSpecifier *
+std::unique_ptr<clang::CXXBaseSpecifier>
ClangASTContext::CreateBaseClassSpecifier(lldb::opaque_compiler_type_t type,
AccessType access, bool is_virtual,
bool base_of_class) {
- if (type)
- return new clang::CXXBaseSpecifier(
- clang::SourceRange(), is_virtual, base_of_class,
- ClangASTContext::ConvertAccessTypeToAccessSpecifier(access),
- getASTContext()->getTrivialTypeSourceInfo(GetQualType(type)),
- clang::SourceLocation());
- return nullptr;
-}
+ if (!type)
+ return nullptr;
-void ClangASTContext::DeleteBaseClassSpecifiers(
- clang::CXXBaseSpecifier **base_classes, unsigned num_base_classes) {
- for (unsigned i = 0; i < num_base_classes; ++i) {
- delete base_classes[i];
- base_classes[i] = nullptr;
- }
+ return llvm::make_unique<clang::CXXBaseSpecifier>(
+ clang::SourceRange(), is_virtual, base_of_class,
+ ClangASTContext::ConvertAccessTypeToAccessSpecifier(access),
+ getASTContext()->getTrivialTypeSourceInfo(GetQualType(type)),
+ clang::SourceLocation());
}
-bool ClangASTContext::SetBaseClassesForClassType(
+bool ClangASTContext::TransferBaseClasses(
lldb::opaque_compiler_type_t type,
- clang::CXXBaseSpecifier const *const *base_classes,
- unsigned num_base_classes) {
- if (type) {
- clang::CXXRecordDecl *cxx_record_decl = GetAsCXXRecordDecl(type);
- if (cxx_record_decl) {
- cxx_record_decl->setBases(base_classes, num_base_classes);
- return true;
- }
- }
- return false;
+ std::vector<std::unique_ptr<clang::CXXBaseSpecifier>> bases) {
+ if (!type)
+ return false;
+ clang::CXXRecordDecl *cxx_record_decl = GetAsCXXRecordDecl(type);
+ if (!cxx_record_decl)
+ return false;
+ std::vector<clang::CXXBaseSpecifier *> raw_bases;
+ raw_bases.reserve(bases.size());
+
+ // Clang will make a copy of them, so it's ok that we pass pointers that we're
+ // about to destroy.
+ for (auto &b : bases)
+ raw_bases.push_back(b.get());
+ cxx_record_decl->setBases(raw_bases.data(), raw_bases.size());
+ return true;
}
bool ClangASTContext::SetObjCSuperClass(
Modified: lldb/trunk/unittests/Symbol/TestClangASTContext.cpp
URL: http://llvm.org/viewvc/llvm-project/lldb/trunk/unittests/Symbol/TestClangASTContext.cpp?rev=345312&r1=345311&r2=345312&view=diff
==============================================================================
--- lldb/trunk/unittests/Symbol/TestClangASTContext.cpp (original)
+++ lldb/trunk/unittests/Symbol/TestClangASTContext.cpp Thu Oct 25 13:44:56 2018
@@ -337,15 +337,19 @@ TEST_F(TestClangASTContext, TestRecordHa
EXPECT_NE(nullptr, non_empty_base_field_decl);
EXPECT_TRUE(ClangASTContext::RecordHasFields(non_empty_base_decl));
+ std::vector<std::unique_ptr<clang::CXXBaseSpecifier>> bases;
+
// Test that a record with no direct fields, but fields in a base returns true
CompilerType empty_derived = m_ast->CreateRecordType(
nullptr, lldb::eAccessPublic, "EmptyDerived", clang::TTK_Struct,
lldb::eLanguageTypeC_plus_plus, nullptr);
ClangASTContext::StartTagDeclarationDefinition(empty_derived);
- CXXBaseSpecifier *non_empty_base_spec = m_ast->CreateBaseClassSpecifier(
- non_empty_base.GetOpaqueQualType(), lldb::eAccessPublic, false, false);
- bool result = m_ast->SetBaseClassesForClassType(
- empty_derived.GetOpaqueQualType(), &non_empty_base_spec, 1);
+ std::unique_ptr<clang::CXXBaseSpecifier> non_empty_base_spec =
+ m_ast->CreateBaseClassSpecifier(non_empty_base.GetOpaqueQualType(),
+ lldb::eAccessPublic, false, false);
+ bases.push_back(std::move(non_empty_base_spec));
+ bool result = m_ast->TransferBaseClasses(empty_derived.GetOpaqueQualType(),
+ std::move(bases));
ClangASTContext::CompleteTagDeclarationDefinition(empty_derived);
EXPECT_TRUE(result);
CXXRecordDecl *empty_derived_non_empty_base_cxx_decl =
@@ -363,10 +367,12 @@ TEST_F(TestClangASTContext, TestRecordHa
nullptr, lldb::eAccessPublic, "EmptyDerived2", clang::TTK_Struct,
lldb::eLanguageTypeC_plus_plus, nullptr);
ClangASTContext::StartTagDeclarationDefinition(empty_derived2);
- CXXBaseSpecifier *non_empty_vbase_spec = m_ast->CreateBaseClassSpecifier(
- non_empty_base.GetOpaqueQualType(), lldb::eAccessPublic, true, false);
- result = m_ast->SetBaseClassesForClassType(empty_derived2.GetOpaqueQualType(),
- &non_empty_vbase_spec, 1);
+ std::unique_ptr<CXXBaseSpecifier> non_empty_vbase_spec =
+ m_ast->CreateBaseClassSpecifier(non_empty_base.GetOpaqueQualType(),
+ lldb::eAccessPublic, true, false);
+ bases.push_back(std::move(non_empty_vbase_spec));
+ result = m_ast->TransferBaseClasses(empty_derived2.GetOpaqueQualType(),
+ std::move(bases));
ClangASTContext::CompleteTagDeclarationDefinition(empty_derived2);
EXPECT_TRUE(result);
CXXRecordDecl *empty_derived_non_empty_vbase_cxx_decl =
@@ -377,9 +383,6 @@ TEST_F(TestClangASTContext, TestRecordHa
empty_derived_non_empty_vbase_cxx_decl, false));
EXPECT_TRUE(
ClangASTContext::RecordHasFields(empty_derived_non_empty_vbase_decl));
-
- delete non_empty_base_spec;
- delete non_empty_vbase_spec;
}
TEST_F(TestClangASTContext, TemplateArguments) {
More information about the lldb-commits
mailing list