Fix handling of CreateTemplateParameterList when there is an empty pack

[shafik]

Summary

When we are creating a ClassTemplateSpecializationDecl in ParseTypeFromDWARF(...) we are not handling the case where variadic pack is empty in the specialization. This patch handles that case and adds a test to prevent future regressions.

[aprantl]

packages/Python/lldbsuite/test/expression_command/radar_47565290/Makefile

1 ↗

(On Diff #183982)

Could you give the directory a more descriptive name instead of radar_47565290? It's fine to mention a rdar:// link in the commit message, but for most people radar numbers are completely opaque.

packages/Python/lldbsuite/test/expression_command/radar_47565290/TestClassTemplateSpecializationParametersHandling.py

2 ↗

(On Diff #183982)

Same here, mentioning the radar in the commit message should be sufficient. This forces us to write better/more complete comments, too :-)

source/Symbol/ClangASTContext.cpp

1562 ↗

(On Diff #183982)

does this get more or less readable if we replace SourceLocation() with {}?

1572 ↗

(On Diff #183982)

Looks like this is the same code as above. Could this be organized differently to avoid duplicating the code? (I didn't look whether these are all different constructors, so maybe this is already as good as it gets)

[aprantl]

packages/Python/lldbsuite/test/expression_command/radar_47565290/TestClassTemplateSpecializationParametersHandling.py

9 ↗

(On Diff #183982)

I don't think these are used

19 ↗

(On Diff #183982)

test_class_template_specialization ?

20 ↗

(On Diff #183982)

This needs to be updated, too.

[aprantl]

packages/Python/lldbsuite/test/expression_command/radar_47565290/TestClassTemplateSpecializationParametersHandling.py

23 ↗

(On Diff #183982)

FYI, Python allows you to write this as self.target, self.process, _, bkpt = without the parentheses, too.
Since you aren't any of the return values, you could just call the function and ignore the return value.

[davide]

packages/Python/lldbsuite/test/expression_command/radar_47565290/TestClassTemplateSpecializationParametersHandling.py

5–6 ↗

(On Diff #183982)

I think this is not needed.

source/Symbol/ClangASTContext.cpp

1572 ↗

(On Diff #183982)

Why this code block became conditional of if (identifier_info) ?
I understand the size check above, but can you elaborate on the need for this?

[teemperor]

packages/Python/lldbsuite/test/expression_command/radar_47565290/main.cpp

1 ↗

(On Diff #183982)

Maybe I miss something, but this could be simpler I think? E.g. like this:

template <typename N, class... P>
struct A {
    int foo() { return 1;}
};

int main() {
  A<int> b;
  return b.foo(); // break here
}

source/Symbol/ClangASTContext.cpp

1558 ↗

(On Diff #183982)

I think !template_param_infos.packed_args->args.empty() is more LLVM-ish.

1562 ↗

(On Diff #183982)

We have SourceLocation() everywhere in clang, so it is at least more consistent this way IMHO.

[shafik]

Addressed comments, including

• Renamed test
• Reduced test size
• Stream-lined code in CreateTemplateParameterList(...)

[shafik]

@aprantl @teemperor @davide thank you for the review, some great catches. I believe I have addressed the comments.

packages/Python/lldbsuite/test/expression_command/radar_47565290/Makefile

1 ↗

(On Diff #183982)

Makes sense, there are a bunch of tests like this. I did not check how old they were though.

packages/Python/lldbsuite/test/expression_command/radar_47565290/TestClassTemplateSpecializationParametersHandling.py

20 ↗

(On Diff #183982)

I will just remove it since the top comment should be sufficient.

packages/Python/lldbsuite/test/expression_command/radar_47565290/main.cpp

1 ↗

(On Diff #183982)

Good catch, I was modeling the code that was the original source of the problem. I had simplified it a lot but yes I could have kept going it appears.

source/Symbol/ClangASTContext.cpp

1562 ↗

(On Diff #183982)

I also think using {} here would obscure more then help, using {} in return statements they make much more sense usually though.

1572 ↗

(On Diff #183982)

I don't think I need it, I thought I did but looking at it over again I don't think that was justified.

1572 ↗

(On Diff #183982)

It was not obvious at first but I was able to simplify a bit.

[teemperor]

LGTM, thanks for the patch!

lldb/trunk/packages/Python/lldbsuite/test/expression_command/class_template_specialization_empty_pack/Makefile

+LEVEL = ../../make
+
+CXX_SOURCES := main.cpp
+
+include $(LEVEL)/Makefile.rules

lldb/trunk/packages/Python/lldbsuite/test/expression_command/class_template_specialization_empty_pack/TestClassTemplateSpecializationParametersHandling.py

+"""
+Test Expression Parser code gen for ClassTemplateSpecializationDecl to insure
+that we generate a TemplateTypeParmDecl in the TemplateParameterList for empty
+variadic packs.
+"""
+
+
+import lldb
+from lldbsuite.test.decorators import *
+from lldbsuite.test.lldbtest import *
+from lldbsuite.test import lldbutil
+
+class TestClassTemplateSpecializationParametersHandling(TestBase):
+
+    mydir = TestBase.compute_mydir(__file__)
+
+    def test_class_template_specialization(self):
+        self.build()
+
+        lldbutil.run_to_source_breakpoint(self, '// break here',
+                lldb.SBFileSpec("main.cpp", False))
+
+        self.expect("expr -u 0 -- b.foo()", substrs=['$0 = 1'])

lldb/trunk/packages/Python/lldbsuite/test/expression_command/class_template_specialization_empty_pack/main.cpp

+template <typename N, class... P>
+struct A {
+    int foo() { return 1;}
+};
+
+int main() {
+  A<int> b;
+  return b.foo(); // break here
+}

lldb/trunk/source/Symbol/ClangASTContext.cpp

for (size_t i = 0; i < num_template_params; ++i) {
     } else {
       template_param_decls.push_back(TemplateTypeParmDecl::Create(
           *ast, decl_context,
           SourceLocation(), SourceLocation(), depth, i, identifier_info,
           is_typename, parameter_pack));
     }
   }
 
-  if (template_param_infos.packed_args &&
-      template_param_infos.packed_args->args.size()) {
+  if (template_param_infos.packed_args) {
     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);
     const bool parameter_pack_true = true;
-    if (IsValueParam(template_param_infos.packed_args->args[0])) {
+
+    if (!template_param_infos.packed_args->args.empty() &&
+        IsValueParam(template_param_infos.packed_args->args[0])) {
       template_param_decls.push_back(NonTypeTemplateParmDecl::Create(
-          *ast, decl_context,
-          SourceLocation(), SourceLocation(), depth, num_template_params,
-          identifier_info,
+          *ast, decl_context, SourceLocation(), SourceLocation(), depth,
+          num_template_params, identifier_info,
           template_param_infos.packed_args->args[0].getIntegralType(),
           parameter_pack_true, nullptr));
     } else {
       template_param_decls.push_back(TemplateTypeParmDecl::Create(
-          *ast, decl_context,
-          SourceLocation(), SourceLocation(), depth, num_template_params,
-          identifier_info,
-          is_typename, parameter_pack_true));
+          *ast, decl_context, SourceLocation(), SourceLocation(), depth,
+          num_template_params, identifier_info, is_typename,
+          parameter_pack_true));
     }
   }
   clang::Expr *const requires_clause = nullptr; // TODO: Concepts
   TemplateParameterList *template_param_list = TemplateParameterList::Create(
       *ast, SourceLocation(), SourceLocation(), template_param_decls,
       SourceLocation(), requires_clause);
   return template_param_list;
 }