[Lldb-commits] [PATCH] D51387: Allow Template argument accessors to automatically unwrap parameter packs

[Shafik Yaghmour via Phabricator via lldb-commits]

shafik added inline comments.


================
Comment at: include/lldb/Symbol/ClangASTContext.h:284
+             ((bool)pack_name == (bool)packed_args) &&
+             (!packed_args || !packed_args->args.empty());
     }
----------------
Is this saying that an empty parameter pack is invalid? We can have an empty parameter pack

https://godbolt.org/z/8zCFz9


================
Comment at: include/lldb/Symbol/ClangASTContext.h:792
+  GetTemplateArgumentKind(lldb::opaque_compiler_type_t type, size_t idx,
+                          bool expand_pack) override;
   CompilerType GetTypeTemplateArgument(lldb::opaque_compiler_type_t type,
----------------
Why not default to `false` here?


================
Comment at: include/lldb/Symbol/ClangASTContext.h:794
   CompilerType GetTypeTemplateArgument(lldb::opaque_compiler_type_t type,
-                                       size_t idx) override;
+                                       size_t idx, bool expand_pack) override;
   llvm::Optional<CompilerType::IntegralTemplateArgument>
----------------
Why not default to `false` here?


================
Comment at: include/lldb/Symbol/ClangASTContext.h:797
+  GetIntegralTemplateArgument(lldb::opaque_compiler_type_t type, size_t idx,
+                              bool expand_pack) override;
 
----------------
Why not default to `false` here?


================
Comment at: source/Symbol/ClangASTContext.cpp:7569
+          if (expand_pack) {
+            auto &pack = template_arg_list[num_args - 1];
+            if (pack.getKind() == clang::TemplateArgument::Pack)
----------------
Do we need to check `num_args != 0`


================
Comment at: source/Symbol/ClangASTContext.cpp:7664
+
+  assert(args.size() &&
+         "We shouldn't have a template specialization without any args");
----------------
The asset before we do math making this assumption i.e. `args.size() - 1`


https://reviews.llvm.org/D51387