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 _______________________________________________ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits