Michael137 accepted this revision. Michael137 added a comment. LGTM
Just had some minor stylistic/documentation suggestions ================ Comment at: lldb/source/API/SBType.cpp:576 lldb::TemplateArgumentKind SBType::GetTemplateArgumentKind(uint32_t idx) { LLDB_INSTRUMENT_VA(this, idx); ---------------- Minor: this could be an opportunity to add documentation for this API in `SBType.h`, mentioning that parameter packs are always expanded ================ Comment at: lldb/source/Plugins/TypeSystem/Clang/TypeSystemClang.cpp:7122 + if (expand_pack && num_args) { + auto &pack = template_arg_list[num_args - 1]; + if (pack.getKind() == clang::TemplateArgument::Pack) ---------------- ================ Comment at: lldb/source/Plugins/TypeSystem/Clang/TypeSystemClang.cpp:7167 + size_t idx, bool expand_pack) { + const auto &args = decl->getTemplateArgs(); + ---------------- Do we use `args.size() - 1` enough times throughout this function that it warrants something like `const auto lastIdx = args.size() - 1;`? ================ Comment at: lldb/source/Plugins/TypeSystem/Clang/TypeSystemClang.cpp:7172 + return nullptr; + + if (idx < args.size() - 1) ---------------- In my opinion this series of checks is subtle enough to warrant some comments. But not a blocker ================ Comment at: lldb/source/Plugins/TypeSystem/Clang/TypeSystemClang.cpp:7175 + return &args[idx]; + + if (!expand_pack || ---------------- ================ Comment at: lldb/source/Plugins/TypeSystem/Clang/TypeSystemClang.cpp:7179 + return idx >= args.size() ? nullptr : &args[idx]; + + const auto &pack = args[args.size() - 1]; ---------------- ================ Comment at: lldb/source/Plugins/TypeSystem/Clang/TypeSystemClang.cpp:7181 + const auto &pack = args[args.size() - 1]; + return &pack.pack_elements()[idx - (args.size() - 1)]; +} ---------------- Do we want a defensive check on 'idx' here? Something along the lines of ``` const auto pack_idx = idx - (args.size() - 1); const auto pack_size = pack.pack_size(); assert (pack_idx < pack_size && "Parameter pack index out-of-bounds"); if (pack_idx >= pack_size) return nullptr; ``` ================ Comment at: lldb/source/Plugins/TypeSystem/Clang/TypeSystemClang.h:325 bool IsValid() const { // Having a pack name but no packed args doesn't make sense, so mark // these template parameters as invalid. ---------------- Minor: this comment applies to the condition on line 330 now. Could move it down? CHANGES SINCE LAST ACTION https://reviews.llvm.org/D51387/new/ https://reviews.llvm.org/D51387 _______________________________________________ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits