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

Reply via email to