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
[email protected]
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
