[PATCH] D73462: [dwarf-5] Support DebugInfo for Defaulted parameters for C++ templates

[David Blaikie via Phabricator via cfe-commits]

dblaikie added inline comments.


================
Comment at: clang/test/CodeGenCXX/debug-info-template-parameter.cpp:30-32
+  foo<int> f1;
+  foo<float> f2;
+  foo<> f3;
----------------
What are these three (6 if you include the fact that each foo has one default template parameter still) testing? Could it use fewer tests.

Perhaps something similar to the IR/LLVM test - one instantiation with a default type parameter and a default non-type parameter, and one instantiation with non-defaults?


================
Comment at: llvm/lib/IR/AsmWriter.cpp:2089
   Printer.printMetadata("type", N->getRawType());
+  Printer.printBool("defaulted", N->isDefault(), /* ShouldSkipFalse*/ false);
   Printer.printMetadata("value", N->getValue(), /* ShouldSkipNull */ false);
----------------
This comment ("ShouldSkipFalse") is incorrect, perhaps "Default=" would be a better comment.


================
Comment at: llvm/test/Assembler/DITemplateParameter.ll:47-51
+; CHECK: 14 = !DITemplateTypeParameter({{.*}}
+!14 = !DITemplateTypeParameter(name: "T", type: !10)
+
+; CHECK: 15 = !DITemplateValueParameter({{.*}}
+!15 = !DITemplateValueParameter(name: "i", type: !10, value: i32 6)
----------------
Probably want to check that these two don't have a "defaulted: true" (or other "defaulted: ") parameter?
 Probably the easiest way is to positively check for the fields that are there:

```
; CHECK: = !DITemplateTypeParameter(name: "T", type: !{{[0-9]*}})
; CHECK: = !DITemplateTypeParameter(name: "i", type: !{{[0-9]*}}, value: i32 6)
```



================
Comment at: llvm/test/DebugInfo/X86/debug-info-template-parameter.ll:80-81
+!13 = !{!14, !15}
+!14 = !DITemplateTypeParameter(name: "T", type: !10, defaulted: false)
+!15 = !DITemplateValueParameter(name: "i", type: !10, defaulted: false, value: i32 6)
+!16 = !DILocation(line: 30, column: 14, scope: !7)
----------------
Could probably remove the "Defaulted: false" values here, since they're the default.


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

https://reviews.llvm.org/D73462