Michael137 created this revision.
Michael137 added reviewers: aprantl, labath.
Herald added a reviewer: shafik.
Herald added a project: All.
Michael137 requested review of this revision.
Herald added a project: LLDB.
Herald added a subscriber: lldb-commits.
This patch makes the members of `TemplateParameterInfos` only accessible
via public APIs. The motivation for this is that
`TemplateParameterInfos` attempts to maintain two vectors in tandem
(`args` for the template arguments and `names` for the corresponding
name). Working with this structure as it's currently designed makes
it easy to run into out-of-bounds accesses later down the line.
This patch proposes to introduce a new
`TemplateParameterInfos::InsertArg` which is the only way to
set the `TemplateArgument` and name of an entry and since we
require both to be specified we maintain the vectors in sync
out-of-the-box.
To avoid adding non-const getters just for unit-tests a new
`TemplateParameterInfosManipulatorForTests` is introduced
that can be used to control internal state from tests.
Repository:
rG LLVM Github Monorepo
https://reviews.llvm.org/D140030
Files:
lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp
lldb/source/Plugins/TypeSystem/Clang/TypeSystemClang.cpp
lldb/source/Plugins/TypeSystem/Clang/TypeSystemClang.h
lldb/unittests/Symbol/TestTypeSystemClang.cpp
Index: lldb/unittests/Symbol/TestTypeSystemClang.cpp
===================================================================
--- lldb/unittests/Symbol/TestTypeSystemClang.cpp
+++ lldb/unittests/Symbol/TestTypeSystemClang.cpp
@@ -470,11 +470,10 @@
TEST_F(TestTypeSystemClang, TemplateArguments) {
TypeSystemClang::TemplateParameterInfos infos;
- infos.names.push_back("T");
- infos.args.push_back(TemplateArgument(m_ast->getASTContext().IntTy));
- infos.names.push_back("I");
+ infos.InsertArg("T", TemplateArgument(m_ast->getASTContext().IntTy));
+
llvm::APSInt arg(llvm::APInt(8, 47));
- infos.args.push_back(TemplateArgument(m_ast->getASTContext(), arg,
+ infos.InsertArg("I", TemplateArgument(m_ast->getASTContext(), arg,
m_ast->getASTContext().IntTy));
// template<typename T, int I> struct foo;
@@ -601,27 +600,26 @@
// This describes a class template *instantiation* from which we will infer
// the structure of the class template.
TypeSystemClang::TemplateParameterInfos infos;
+ auto manipulator = infos.GetManipulatorForTests();
// Test an empty template parameter list: <>
ExpectNewTemplate("<>", infos);
// Test that <typename T> with T = int creates a new template.
- infos.names = {"T"};
- infos.args = {TemplateArgument(m_ast->getASTContext().IntTy)};
+ infos.InsertArg("T", TemplateArgument(m_ast->getASTContext().IntTy));
ClassTemplateDecl *single_type_arg = ExpectNewTemplate("<typename T>", infos);
// Test that changing the parameter name doesn't create a new class template.
- infos.names = {"A"};
+ manipulator.SetNames({"A"});
ExpectReusedTemplate("<typename A> (A = int)", infos, single_type_arg);
// Test that changing the used type doesn't create a new class template.
- infos.args = {TemplateArgument(m_ast->getASTContext().FloatTy)};
+ manipulator.SetArgs({TemplateArgument(m_ast->getASTContext().FloatTy)});
ExpectReusedTemplate("<typename A> (A = float)", infos, single_type_arg);
// Test that <typename A, signed char I> creates a new template with A = int
// and I = 47;
- infos.names.push_back("I");
- infos.args.push_back(TemplateArgument(m_ast->getASTContext(),
+ infos.InsertArg("I", TemplateArgument(m_ast->getASTContext(),
llvm::APSInt(llvm::APInt(8, 47)),
m_ast->getASTContext().SignedCharTy));
ClassTemplateDecl *type_and_char_value =
@@ -629,31 +627,31 @@
// Change the value of the I parameter to 123. The previously created
// class template should still be reused.
- infos.args.pop_back();
- infos.args.push_back(TemplateArgument(m_ast->getASTContext(),
- llvm::APSInt(llvm::APInt(8, 123)),
- m_ast->getASTContext().SignedCharTy));
+ manipulator.PopBackArg();
+ manipulator.PushBackArg(TemplateArgument(
+ m_ast->getASTContext(), llvm::APSInt(llvm::APInt(8, 123)),
+ m_ast->getASTContext().SignedCharTy));
ExpectReusedTemplate("<typename A, signed char I> (I = 123)", infos,
type_and_char_value);
// Change the type of the I parameter to int so we have <typename A, int I>.
// The class template from above can't be reused.
- infos.args.pop_back();
- infos.args.push_back(TemplateArgument(m_ast->getASTContext(),
- llvm::APSInt(llvm::APInt(32, 47)),
- m_ast->getASTContext().IntTy));
+ manipulator.PopBackArg();
+ manipulator.PushBackArg(TemplateArgument(m_ast->getASTContext(),
+ llvm::APSInt(llvm::APInt(32, 47)),
+ m_ast->getASTContext().IntTy));
ExpectNewTemplate("<typename A, int I> (I = 123)", infos);
// Test a second type parameter will also cause a new template to be created.
// We now have <typename A, int I, typename B>.
- infos.names.push_back("B");
- infos.args.push_back(TemplateArgument(m_ast->getASTContext().IntTy));
+ manipulator.PushBackName("B");
+ manipulator.PushBackArg(TemplateArgument(m_ast->getASTContext().IntTy));
ClassTemplateDecl *type_and_char_value_and_type =
ExpectNewTemplate("<typename A, int I, typename B>", infos);
// Remove all the names from the parameters which shouldn't influence the
// way the templates get merged.
- infos.names = {"", "", ""};
+ manipulator.SetNames({"", "", ""});
ExpectReusedTemplate("<typename, int, typename>", infos,
type_and_char_value_and_type);
}
@@ -662,62 +660,67 @@
// The same as FindExistingTemplates but for templates with parameter packs.
TypeSystemClang::TemplateParameterInfos infos;
- infos.packed_args =
- std::make_unique<TypeSystemClang::TemplateParameterInfos>();
- infos.packed_args->names = {"", ""};
- infos.packed_args->args = {TemplateArgument(m_ast->getASTContext().IntTy),
- TemplateArgument(m_ast->getASTContext().IntTy)};
+ infos.SetParameterPack(
+ std::make_unique<TypeSystemClang::TemplateParameterInfos>());
+
+ infos.InsertArg("", TemplateArgument(m_ast->getASTContext().IntTy));
+ infos.InsertArg("", TemplateArgument(m_ast->getASTContext().IntTy));
+
ClassTemplateDecl *type_pack =
ExpectNewTemplate("<typename ...> (int, int)", infos);
// Special case: An instantiation for a parameter pack with no values fits
// to whatever class template we find. There isn't enough information to
// do an actual comparison here.
- infos.packed_args =
- std::make_unique<TypeSystemClang::TemplateParameterInfos>();
+ infos.SetParameterPack(
+ std::make_unique<TypeSystemClang::TemplateParameterInfos>());
ExpectReusedTemplate("<...> (no values in pack)", infos, type_pack);
// Change the type content of pack type values.
- infos.packed_args->names = {"", ""};
- infos.packed_args->args = {TemplateArgument(m_ast->getASTContext().IntTy),
- TemplateArgument(m_ast->getASTContext().LongTy)};
+ infos.GetParameterPack().InsertArg(
+ "", TemplateArgument(m_ast->getASTContext().IntTy));
+ infos.GetParameterPack().InsertArg(
+ "", TemplateArgument(m_ast->getASTContext().LongTy));
+
ExpectReusedTemplate("<typename ...> (int, long)", infos, type_pack);
// Change the number of pack values.
- infos.packed_args->args = {TemplateArgument(m_ast->getASTContext().IntTy)};
+ infos.GetParameterPack().GetManipulatorForTests().SetArgs(
+ {TemplateArgument(m_ast->getASTContext().IntTy)});
ExpectReusedTemplate("<typename ...> (int)", infos, type_pack);
// The names of the pack values shouldn't matter.
- infos.packed_args->names = {"A", "B"};
+ infos.GetParameterPack().GetManipulatorForTests().SetNames({"A", "B"});
ExpectReusedTemplate("<typename ...> (int)", infos, type_pack);
// Changing the kind of template argument will create a new template.
- infos.packed_args->args = {TemplateArgument(m_ast->getASTContext(),
- llvm::APSInt(llvm::APInt(32, 1)),
- m_ast->getASTContext().IntTy)};
+ infos.GetParameterPack().GetManipulatorForTests().SetArgs({TemplateArgument(
+ m_ast->getASTContext(), llvm::APSInt(llvm::APInt(32, 1)),
+ m_ast->getASTContext().IntTy)});
ClassTemplateDecl *int_pack = ExpectNewTemplate("<int ...> (int = 1)", infos);
// Changing the value of integral parameters will not create a new template.
- infos.packed_args->args = {TemplateArgument(
+ infos.GetParameterPack().GetManipulatorForTests().SetArgs({TemplateArgument(
m_ast->getASTContext(), llvm::APSInt(llvm::APInt(32, 123)),
- m_ast->getASTContext().IntTy)};
+ m_ast->getASTContext().IntTy)});
ExpectReusedTemplate("<int ...> (int = 123)", infos, int_pack);
// Changing the integral type will create a new template.
- infos.packed_args->args = {TemplateArgument(m_ast->getASTContext(),
- llvm::APSInt(llvm::APInt(64, 1)),
- m_ast->getASTContext().LongTy)};
+ infos.GetParameterPack().GetManipulatorForTests().SetArgs({TemplateArgument(
+ m_ast->getASTContext(), llvm::APSInt(llvm::APInt(64, 1)),
+ m_ast->getASTContext().LongTy)});
ExpectNewTemplate("<long ...> (long = 1)", infos);
// Prependinding a non-pack parameter will create a new template.
- infos.names = {"T"};
- infos.args = {TemplateArgument(m_ast->getASTContext().IntTy)};
+ infos.GetManipulatorForTests().SetNames({"T"});
+ infos.GetManipulatorForTests().SetArgs(
+ {TemplateArgument(m_ast->getASTContext().IntTy)});
ExpectNewTemplate("<typename T, long...> (T = int, long = 1)", infos);
}
TEST_F(TestTypeSystemClang, OnlyPackName) {
TypeSystemClang::TemplateParameterInfos infos;
- infos.pack_name = "A";
+ infos.SetPackName("A");
EXPECT_FALSE(infos.IsValid());
}
Index: lldb/source/Plugins/TypeSystem/Clang/TypeSystemClang.h
===================================================================
--- lldb/source/Plugins/TypeSystem/Clang/TypeSystemClang.h
+++ lldb/source/Plugins/TypeSystem/Clang/TypeSystemClang.h
@@ -330,6 +330,40 @@
class TemplateParameterInfos {
public:
+ /// Class to be used only within unit-tests for manipulating
+ /// private state of \ref TemplateParameterInfos.
+ class TemplateParamInfosManipulatorForTests {
+ public:
+ TemplateParamInfosManipulatorForTests(TemplateParameterInfos &infos)
+ : m_infos(infos) {}
+
+ void SetArgs(llvm::SmallVector<clang::TemplateArgument> args) {
+ m_infos.args = std::move(args);
+ }
+
+ void SetNames(llvm::SmallVector<char const *> names) {
+ m_infos.names = std::move(names);
+ }
+
+ void PopBackArg() { m_infos.args.pop_back(); }
+
+ void PushBackArg(clang::TemplateArgument arg) {
+ m_infos.args.emplace_back(std::move(arg));
+ }
+
+ void PushBackName(char const *name) { m_infos.names.push_back(name); }
+
+ private:
+ TemplateParameterInfos &m_infos;
+ };
+
+ /// Return an object that has access to the internal state
+ /// of this TemplateParameterInfos instance. Only to be used
+ /// in tests.
+ auto GetManipulatorForTests() {
+ return TemplateParamInfosManipulatorForTests(*this);
+ }
+
bool IsValid() const {
// Having a pack name but no packed args doesn't make sense, so mark
// these template parameters as invalid.
@@ -339,8 +373,57 @@
(!packed_args || !packed_args->packed_args);
}
+ bool IsEmpty() const { return args.empty(); }
+ size_t Size() const { return args.size(); }
+
+ llvm::ArrayRef<clang::TemplateArgument> GetArgs() const { return args; }
+
+ llvm::ArrayRef<char const *> GetNames() const { return names; }
+
+ clang::TemplateArgument const &Front() const {
+ assert(!args.empty());
+ return args.front();
+ }
+
+ void InsertArg(char const *name, clang::TemplateArgument arg) {
+ args.emplace_back(std::move(arg));
+ names.push_back(name);
+ }
+
+ // Parameter pack related
+
bool hasParameterPack() const { return static_cast<bool>(packed_args); }
+ TemplateParameterInfos const &GetParameterPack() const {
+ assert(packed_args != nullptr);
+ return *packed_args;
+ }
+
+ TemplateParameterInfos &GetParameterPack() {
+ assert(packed_args != nullptr);
+ return *packed_args;
+ }
+
+ llvm::ArrayRef<clang::TemplateArgument> GetParameterPackArgs() const {
+ assert(packed_args != nullptr);
+ return packed_args->GetArgs();
+ }
+
+ bool HasPackName() const { return pack_name && pack_name[0]; }
+
+ llvm::StringRef GetPackName() const {
+ assert(HasPackName());
+ return pack_name;
+ }
+
+ void SetPackName(char const *name) { pack_name = name; }
+
+ void SetParameterPack(std::unique_ptr<TemplateParameterInfos> args) {
+ packed_args = std::move(args);
+ }
+
+ private:
+ ///< Element 'names[i]' holds the template argument name of 'args[i]'
llvm::SmallVector<const char *, 2> names;
llvm::SmallVector<clang::TemplateArgument, 2> args;
Index: lldb/source/Plugins/TypeSystem/Clang/TypeSystemClang.cpp
===================================================================
--- lldb/source/Plugins/TypeSystem/Clang/TypeSystemClang.cpp
+++ lldb/source/Plugins/TypeSystem/Clang/TypeSystemClang.cpp
@@ -1377,18 +1377,21 @@
const bool parameter_pack = false;
const bool is_typename = false;
const unsigned depth = 0;
- const size_t num_template_params = template_param_infos.args.size();
+ const size_t num_template_params = template_param_infos.Size();
DeclContext *const decl_context =
ast.getTranslationUnitDecl(); // Is this the right decl context?,
+
+ auto const &args = template_param_infos.GetArgs();
+ auto const &names = template_param_infos.GetNames();
for (size_t i = 0; i < num_template_params; ++i) {
- const char *name = template_param_infos.names[i];
+ const char *name = names[i];
IdentifierInfo *identifier_info = nullptr;
if (name && name[0])
identifier_info = &ast.Idents.get(name);
- if (IsValueParam(template_param_infos.args[i])) {
- QualType template_param_type =
- template_param_infos.args[i].getIntegralType();
+ TemplateArgument const &targ = args[i];
+ if (IsValueParam(targ)) {
+ QualType template_param_type = targ.getIntegralType();
template_param_decls.push_back(NonTypeTemplateParmDecl::Create(
ast, decl_context, SourceLocation(), SourceLocation(), depth, i,
identifier_info, template_param_type, parameter_pack,
@@ -1400,16 +1403,16 @@
}
}
- if (template_param_infos.packed_args) {
+ if (template_param_infos.hasParameterPack()) {
IdentifierInfo *identifier_info = nullptr;
- if (template_param_infos.pack_name && template_param_infos.pack_name[0])
- identifier_info = &ast.Idents.get(template_param_infos.pack_name);
+ if (template_param_infos.HasPackName())
+ identifier_info = &ast.Idents.get(template_param_infos.GetPackName());
const bool parameter_pack_true = true;
- if (!template_param_infos.packed_args->args.empty() &&
- IsValueParam(template_param_infos.packed_args->args[0])) {
+ if (!template_param_infos.GetParameterPack().IsEmpty() &&
+ IsValueParam(template_param_infos.GetParameterPack().Front())) {
QualType template_param_type =
- template_param_infos.packed_args->args[0].getIntegralType();
+ template_param_infos.GetParameterPack().Front().getIntegralType();
template_param_decls.push_back(NonTypeTemplateParmDecl::Create(
ast, decl_context, SourceLocation(), SourceLocation(), depth,
num_template_params, identifier_info, template_param_type,
@@ -1465,8 +1468,8 @@
void TypeSystemClang::CreateFunctionTemplateSpecializationInfo(
FunctionDecl *func_decl, clang::FunctionTemplateDecl *func_tmpl_decl,
const TemplateParameterInfos &infos) {
- TemplateArgumentList *template_args_ptr =
- TemplateArgumentList::CreateCopy(func_decl->getASTContext(), infos.args);
+ TemplateArgumentList *template_args_ptr = TemplateArgumentList::CreateCopy(
+ func_decl->getASTContext(), infos.GetArgs());
func_decl->setFunctionTemplateSpecialization(func_tmpl_decl,
template_args_ptr, nullptr);
@@ -1537,7 +1540,7 @@
// The found template needs to have compatible non-pack template arguments.
// E.g., ensure that <typename, typename> != <typename>.
// The pack parameters are compared later.
- if (non_pack_params != instantiation_values.args.size())
+ if (non_pack_params != instantiation_values.Size())
return false;
// Ensure that <typename...> != <typename>.
@@ -1548,14 +1551,15 @@
// parameter value. The special case of having an empty parameter pack value
// always fits to a pack parameter.
// E.g., ensure that <int...> != <typename...>.
- if (pack_parameter && !instantiation_values.packed_args->args.empty() &&
+ if (pack_parameter && !instantiation_values.GetParameterPack().IsEmpty() &&
!TemplateParameterAllowsValue(
- *pack_parameter, instantiation_values.packed_args->args.front()))
+ *pack_parameter, instantiation_values.GetParameterPack().Front()))
return false;
// Compare all the non-pack parameters now.
// E.g., ensure that <int> != <long>.
- for (const auto pair : llvm::zip_first(instantiation_values.args, params)) {
+ for (const auto pair :
+ llvm::zip_first(instantiation_values.GetArgs(), params)) {
const TemplateArgument &passed_arg = std::get<0>(pair);
NamedDecl *found_param = std::get<1>(pair);
if (!TemplateParameterAllowsValue(found_param, passed_arg))
@@ -1668,13 +1672,14 @@
const TemplateParameterInfos &template_param_infos) {
ASTContext &ast = getASTContext();
llvm::SmallVector<clang::TemplateArgument, 2> args(
- template_param_infos.args.size() +
- (template_param_infos.packed_args ? 1 : 0));
- std::copy(template_param_infos.args.begin(), template_param_infos.args.end(),
- args.begin());
- if (template_param_infos.packed_args) {
+ template_param_infos.Size() +
+ (template_param_infos.hasParameterPack() ? 1 : 0));
+
+ auto const &orig_args = template_param_infos.GetArgs();
+ std::copy(orig_args.begin(), orig_args.end(), args.begin());
+ if (template_param_infos.hasParameterPack()) {
args[args.size() - 1] = TemplateArgument::CreatePackCopy(
- ast, template_param_infos.packed_args->args);
+ ast, template_param_infos.GetParameterPackArgs());
}
ClassTemplateSpecializationDecl *class_template_specialization_decl =
ClassTemplateSpecializationDecl::CreateDeserialized(ast, 0);
Index: lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp
===================================================================
--- lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp
+++ lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp
@@ -1997,14 +1997,14 @@
switch (tag) {
case DW_TAG_GNU_template_parameter_pack: {
- template_param_infos.packed_args =
- std::make_unique<TypeSystemClang::TemplateParameterInfos>();
+ template_param_infos.SetParameterPack(
+ std::make_unique<TypeSystemClang::TemplateParameterInfos>());
for (DWARFDIE child_die : die.children()) {
- if (!ParseTemplateDIE(child_die, *template_param_infos.packed_args))
+ if (!ParseTemplateDIE(child_die, template_param_infos.GetParameterPack()))
return false;
}
if (const char *name = die.GetName()) {
- template_param_infos.pack_name = name;
+ template_param_infos.SetPackName(name);
}
return true;
}
@@ -2061,31 +2061,30 @@
if (!is_template_template_argument) {
bool is_signed = false;
- if (name && name[0])
- template_param_infos.names.push_back(name);
- else
- template_param_infos.names.push_back(nullptr);
-
// Get the signed value for any integer or enumeration if available
clang_type.IsIntegerOrEnumerationType(is_signed);
+ if (name && !name[0])
+ name = nullptr;
+
if (tag == DW_TAG_template_value_parameter && uval64_valid) {
llvm::Optional<uint64_t> size = clang_type.GetBitSize(nullptr);
if (!size)
return false;
llvm::APInt apint(*size, uval64, is_signed);
- template_param_infos.args.push_back(
+ template_param_infos.InsertArg(
+ name,
clang::TemplateArgument(ast, llvm::APSInt(apint, !is_signed),
ClangUtil::GetQualType(clang_type)));
} else {
- template_param_infos.args.push_back(
+ template_param_infos.InsertArg(
+ name,
clang::TemplateArgument(ClangUtil::GetQualType(clang_type)));
}
} else {
auto *tplt_type = m_ast.CreateTemplateTemplateParmDecl(template_name);
- template_param_infos.names.push_back(name);
- template_param_infos.args.push_back(
- clang::TemplateArgument(clang::TemplateName(tplt_type)));
+ template_param_infos.InsertArg(
+ name, clang::TemplateArgument(clang::TemplateName(tplt_type)));
}
}
}
@@ -2119,10 +2118,12 @@
break;
}
}
- return template_param_infos.args.size() ==
- template_param_infos.names.size() &&
- (!template_param_infos.args.empty() ||
- template_param_infos.packed_args);
+
+ assert(template_param_infos.GetArgs().size() ==
+ template_param_infos.GetNames().size());
+
+ return !template_param_infos.IsEmpty() ||
+ template_param_infos.hasParameterPack();
}
bool DWARFASTParserClang::CompleteRecordType(const DWARFDIE &die,
_______________________________________________
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
