[PATCH] D54487: Implement llvm.commandline named metadata

[Paul Robinson via Phabricator via llvm-commits]

probinson added a comment.

Does the verifier bail out after the first failure?  If not, you can combine the failure tests all into one test.



================
Comment at: lib/CodeGen/AsmPrinter/AsmPrinter.cpp:2021
+    if (!NMD->getNumOperands())
+      return;
+
----------------
You can reduce some indentation by consistently using the early-return idiom:
```
const NamedMDNode *NMD = ...;
if (!NMD || !NMD->getNumOperands())
  return;
// rest of function
```


================
Comment at: test/Linker/commandline.ll:8
+; CHECK-DAG: "compiler -v2"
+; CHECK-DAG: "compiler -v3"
+
----------------
You can make sure that each of the strings has one of the expected MD numbers:
```
CHECK-DAG: !{{[0-2]}} = !{!"compiler -v1"}
```
etc.


================
Comment at: test/Verifier/commandline-meta1.ll:7
+!llvm.commandline = !{!0, !1}
+!0 = !{!"commandline string"}
+!1 = !{!"string1", !"string2"}
----------------
I think it would be ok (and more obvious) if you omit the valid entry.


================
Comment at: test/Verifier/commandline-meta2.ll:9
+!1 = !{!"str2"}
+!2 = !{!"str3"}
+!3 = !{i32 1}
----------------
Simpler and more obvious if you omit the valid entries.


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

https://reviews.llvm.org/D54487