[PATCH] D34063: [ThinLTO] YAML traits for module summaries

[Teresa Johnson via Phabricator via llvm-commits]

tejohnson added a comment.

Looks really close, just a few minor suggestions/nits left.



================
Comment at: include/llvm/IR/ModuleSummaryIndexYAML.h:400
+        }
+        if (dyn_cast<GlobalVarSummary>(Sum.get()))
+          GVSums.push_back(GlobalVarSummaryYaml{GVSumY});
----------------
You can use "isa" instead of "dyn_cast" here now


================
Comment at: include/llvm/IR/ModuleSummaryIndexYAML.h:426
+    if (M.Mod) {
+      for (GlobalValue &V : M.Mod->global_values()) {
+        NameMap.insert(std::make_pair(V.getGUID(), V.getName()));
----------------
Minor nit: prefer no braces around single statement for and if bodies.


================
Comment at: include/llvm/Support/YAMLTraits.h:659
+  void addComment(Comment C) {
+    if (outputting() && !C.Str.empty()) {
+      comment(C.Str, C.NewLine);
----------------
Ditto on no braces preferred. 


================
Comment at: test/tools/llvm-lto2/X86/dump-summary-yaml.ll:14
+; CHECK:       Calls:
+; CHECK:         - GUID:
+; CHECK:           Hotness:         Unknown
----------------
For the lines that have names, can you check for the names as well? You could either put in the expected GUID value, or use a regex to accept any GUID #. The FileCheck regex format for matching anything is: {{.*}}


https://reviews.llvm.org/D34063