[PATCH] D74470: Seperated DIBasicType DIFlags to DIBTFlags.

[Adrian Prantl via Phabricator via llvm-commits]

aprantl added a comment.

Thanks! This looks quite good already, I just have a few inline comments.



================
Comment at: llvm/include/llvm/IR/DebugInfoMetadata.h:692
+public:
+  /// Debug BasciType flags.
+  enum DIBTFlags : uint32_t {
----------------
`Basic`


================
Comment at: llvm/lib/AsmParser/LLParser.cpp:4171
+template <>
+bool LLParser::ParseMDField(LocTy Loc, StringRef Name, DIBTFlagField &Result) {
+
----------------
Could the bulk of the implementation of this function be shared with the function that parses DIFlag and DISPFlag?


================
Comment at: llvm/lib/Bitcode/Reader/MetadataLoader.cpp:1300
+
+    // BasicType Specific flags in DIFlag Moved to DIBTFlags
+    const unsigned DIFlagBigEndian = (1 << 27);
----------------
`// BasicType-specific flags in DIFlag were moved to DIBTFlags.`


================
Comment at: llvm/lib/Bitcode/Reader/MetadataLoader.cpp:1302
+    const unsigned DIFlagBigEndian = (1 << 27);
+    const unsigned DIFlagLittleEndian = (1 << 28);
+    if (Record.size() > 6) {
----------------
Thanks!


================
Comment at: llvm/test/Bitcode/DIBasicType.ll:27
+!8 = !DIBasicType(name: "int", size: 32, encoding: DW_ATE_signed, btFlags: DIBTFlagBigEndian)
+!9 = !DIBasicType(name: "int", size: 32, encoding: DW_ATE_signed, btFlags: DIBTFlagLittleEndian)
+!10 = !{i32 7, !"Dwarf Version", i32 4}
----------------
Assuming that the IR in this file is the input for `DIBasicType.ll.bc`, I would have expected this to use the *old* format to test that the old format can be upgraded to the new format. The assembler roundtrip test `debug-info.ll` is already testing that the new format works.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D74470