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

[Michael Buch via Phabricator via lldb-commits]

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