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

[David Blaikie via Phabricator via llvm-commits]

dblaikie added inline comments.


================
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)
----------------
dblaikie wrote:
> 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?".
I see that the actual DWARFv5 feature is "DW_AT_default_value" - so I guess sticking with that is good, even though it's not a great name (& indeed elsewhere in DWARF this same attribute name is used with a value that represents the default value, not a boolean about whether the default value is elsewhere... :/)

Perhaps we should push the name further up then - into the API, (IsDefaultValue member, maybe make the accessor "isDefaultValue" rather than "getDefaultValue" to avoid some of the ambiguity/confusion, etc).


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

https://reviews.llvm.org/D73462