[Lldb-commits] [PATCH] D57363: Fix handling of CreateTemplateParameterList when there is an empty pack

[Shafik Yaghmour via Phabricator via lldb-commits]

shafik added a comment.

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



================
Comment at: packages/Python/lldbsuite/test/expression_command/radar_47565290/Makefile:1
+LEVEL = ../../make
+
----------------
aprantl wrote:
> 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.
Makes sense, there are a bunch of tests like this. I did not check how old they were though.


================
Comment at: packages/Python/lldbsuite/test/expression_command/radar_47565290/TestClassTemplateSpecializationParametersHandling.py:20
+    def test_with_run_command(self):
+        """Test that that file and class static variables display correctly."""
+        self.build()
----------------
aprantl wrote:
> This needs to be updated, too.
I will just remove it since the top comment should be sufficient.


================
Comment at: packages/Python/lldbsuite/test/expression_command/radar_47565290/main.cpp:1
+template <typename N, typename P, class... O>
+class A {
----------------
teemperor wrote:
> 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
> }
> ```
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.


================
Comment at: source/Symbol/ClangASTContext.cpp:1562
+        template_param_decls.push_back(NonTypeTemplateParmDecl::Create(
+            *ast, decl_context, SourceLocation(), SourceLocation(), depth,
+            num_template_params, identifier_info,
----------------
teemperor wrote:
> aprantl wrote:
> > does this get more or less readable if we replace `SourceLocation()` with `{}`?
> We have `SourceLocation()` everywhere in clang, so it is at least more consistent this way IMHO.
I also think using `{}` here would obscure more then help, using `{}` in return statements they make much more sense usually though.


================
Comment at: source/Symbol/ClangASTContext.cpp:1572
+      }
+    } else if (identifier_info) {
       template_param_decls.push_back(TemplateTypeParmDecl::Create(
----------------
davide wrote:
> aprantl wrote:
> > 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)
> 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?
I don't think I need it, I thought I did but looking at it over again I don't think that was justified. 


================
Comment at: source/Symbol/ClangASTContext.cpp:1572
+      }
+    } else if (identifier_info) {
       template_param_decls.push_back(TemplateTypeParmDecl::Create(
----------------
shafik wrote:
> davide wrote:
> > aprantl wrote:
> > > 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)
> > 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?
> I don't think I need it, I thought I did but looking at it over again I don't think that was justified. 
It was not obvious at first but I was able to simplify a bit.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D57363/new/

https://reviews.llvm.org/D57363