[PATCH] D41695: [Metadata] Extend 'count' field of DISubrange to take a metadata node

[Adrian Prantl via Phabricator via llvm-commits]

aprantl accepted this revision.
aprantl added a comment.
This revision is now accepted and ready to land.

I have a few more comments inline, with the exception of the one in MetadataLoader, all just minor nitpicks.

LGTM with those changes addressed!
Thanks!



================
Comment at: lib/AsmParser/LLParser.cpp:3892
+                            MDSignedOrMDField &Result) {
+  // Try to parse a signed int
+  if (Lex.getKind() == lltok::APSInt) {
----------------
missing `.` at the end


================
Comment at: lib/AsmParser/LLParser.cpp:3902
+
+  // Otherwise, try to parse as an MDField
+  MDField Res = Result.B;
----------------
same here.


================
Comment at: lib/AsmParser/LLParser.cpp:4067
+        DISubrange, (Context, count.getMDSignedValue(), lowerBound.Val));
+  else if (count.isMDField()) {
+    Result = GET_OR_DISTINCT(
----------------
the application of `{` is inconsistent in this nested if-statements.


================
Comment at: lib/Bitcode/Reader/MetadataLoader.cpp:1178
+    Metadata *Val = nullptr;
+    switch (Record[0] >> 1) {
+    case 0:
----------------
Please add a comment here explaining the difference between version 0 and version 0.


================
Comment at: lib/Bitcode/Reader/MetadataLoader.cpp:1188
+    default:
+      llvm_unreachable("Version of DISubrange not supported");
+    }
----------------
This should be `return error("...")`


https://reviews.llvm.org/D41695