[PATCH] D101835: [llvm] llvm-tapi-diff

[Cyndy Ishida via Phabricator via llvm-commits]

cishida added inline comments.


================
Comment at: llvm/include/llvm/TextAPI/Target.h:63
 
+std::string getTargetTriple(Target Targ);
+
----------------
nit: s/getTargetTriple/getTripleName or getTargetTripleName 
Triple is already a type so this could be conflicting if there are similar functions that have that return type


================
Comment at: llvm/tools/llvm-tapi-diff/DiffEngine.h:132
+    switch (Flag) {
+    case MachO::SymbolFlags::None:
+      return "None";
----------------
I'd prefer this emit an empty string 


================
Comment at: llvm/tools/llvm-tapi-diff/llvm-tapi-diff.cpp:79
+    }
+  return 1;
+}
----------------
This path should never be possible, right? Could we instead error out if `Bin*` can't be casted & early return. 

and have this be `return Engine.compareFiles()`


================
Comment at: llvm/tools/llvm-tapi-diff/llvm-tapi-diff.cpp:1
+//===-- llvm-nm-diff.cpp - symbol table comparator command-line driver ---*- C++
+//-*-===//
----------------
Please update these header descriptions 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D101835