[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: llvm/include/llvm/IR/DebugInfoMetadata.h:2134
 protected:
+  bool isDefault;
+
----------------
Rename this (& the ctor/other parameters) to "IsDefault" to conform to LLVM's naming conventions ( https://llvm.org/docs/CodingStandards.html#name-types-functions-variables-and-enumerators-properly )




================
Comment at: llvm/lib/AsmParser/LLParser.cpp:4858
 ///   ::= !DITemplateValueParameter(tag: DW_TAG_template_value_parameter,
-///                                 name: "V", type: !1, value: i32 7)
+///                                 name: "V", type: !1, defaultValue: "false",
+///                                 value: i32 7)
----------------
defaultValue should probably be rendered/encoded as a boolean, rather than a string?

(& "defaultValue" might be a misleading name (if the DITemplateValueParameter was a boolean non type template parameter, then "defaultValue: true" could look like the default value is true, not that the "value: "parameter is the default value... if that makes sense) - perhaps "defaulted" would read more easily ("defaulted: true" or "defaulted: false"))

Perhaps just "default" though that still feels a bit ambiguous between "is this the default value itself, or is it specifying that the "value:" field is the default value?".


================
Comment at: llvm/lib/Bitcode/Writer/BitcodeWriter.cpp:1808-1809
   Record.push_back(VE.getMetadataOrNullID(N->getType()));
+  if (M.getDwarfVersion() >= 5)
+    Record.push_back(N->getDefault());
   Record.push_back(VE.getMetadataOrNullID(N->getValue()));
----------------
I don't think we should be using the DWARF version to decide on the schema - there's no other use of that technique in the parsing/writing code & I can think of some ways it might go poorly.

Better to encode it & it can be dropped during actual DWARF emission in the backend if the version doesn't support it.


================
Comment at: llvm/test/DebugInfo/X86/debug-info-template-parameter.ll:1
+; RUN: %llc_dwarf  %s -filetype=obj -o - | llvm-dwarfdump -v - | FileCheck %s
+
----------------
An implicit-check-not of DW_TAG and of DW_AT_default_value would probably help make the test better constrained/less likely to pass accidentally/in unintended ways.

(though the DW_TAG check not might be too much/add too much noise outside the types intended to be checked (you'd need to add every DW_TAG produced by the test... maybe there's a way to simplify the uses of the types so the usage doesn't need to create many more tags... )


================
Comment at: llvm/test/DebugInfo/X86/debug-info-template-parameter.ll:5
+
+;template <typename T = char, typename T1 = int>
+;class foo {
----------------
Perhaps you could swap one of these defaulted template parameters to be a non-type template parameter to broaden the demonstration a bit?

Or, alternatively, does having two parameters here especially broaden the test coverage in some way? Or would one be sufficient? 


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

https://reviews.llvm.org/D73462