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 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
