friss marked 2 inline comments as done.
friss added inline comments.
================
Comment at: include/lldb/Symbol/ClangASTContext.h:284
+ ((bool)pack_name == (bool)packed_args) &&
+ (!packed_args || !packed_args->args.empty());
}
----------------
friss wrote:
> shafik wrote:
> > Is this saying that an empty parameter pack is invalid? We can have an
> > empty parameter pack
> >
> > https://godbolt.org/z/8zCFz9
> That's a good question. I need to check what the DWARF looks like for an
> empty parameter pack. I would expect it not to be mentioned at all, but if
> it's there we need to support it.
This was a good catch. The fact that there is an empty pack was recorded in the
debug info and this made us consider the type as invalid. I've added a test to
cover this.
================
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,
----------------
shafik wrote:
> Why not default to `false` here?
I added the defaults at the top to keep the semantics for the existing code,
but it turns out there's no code using those interfaces that I haven't updated.
I'll remove the defaults from everywhere.
================
Comment at: source/Symbol/ClangASTContext.cpp:7664
+
+ assert(args.size() &&
+ "We shouldn't have a template specialization without any args");
----------------
shafik wrote:
> The asset before we do math making this assumption i.e. `args.size() - 1`
The assert is also wrong because of the empty pack issue. I'll update the patch.
https://reviews.llvm.org/D51387
_______________________________________________
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
