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

[Jonas Devlieghere via Phabricator via llvm-commits]

JDevlieghere added a comment.

A few small nits and this should be ready to go.



================
Comment at: llvm/tools/llvm-tapi-diff/DiffEngine.cpp:383-386
+    for (auto Doc1 : IFLHS->documents()) {
+      auto Pair = llvm::find_if(IFRHS->documents(), [&](const auto &Doc2) {
+        return (Doc1->getInstallName() == Doc2->getInstallName());
+      });
----------------
`LHS`? `DocLHS`?


================
Comment at: llvm/tools/llvm-tapi-diff/DiffEngine.cpp:457
+  std::string Indent = std::string(IndentCounter, '\t');
+  raw_ostream &OS = outs();
+  for (auto &Attr : Diffs) {
----------------
This line got me thinking that it might be nice to make the `raw_ostream` an argument to `printDifferences` and do the same for `compareFiles` below. That aligns nicely with LLVM's library design, where the actual implementation (the DiffEngine) can remain unaware of where its output is being printed while centralize the fact that it's going to stdout in `llvm-tapi-diff.cpp`. 


================
Comment at: llvm/tools/llvm-tapi-diff/llvm-tapi-diff.cpp:29-32
+cl::opt<std::string> InputFileNameLHS(cl::Positional, cl::desc("<First File>"),
+                                      cl::cat(NMCat));
+cl::opt<std::string> InputFileNameRHS(cl::Positional, cl::desc("<Second File>"),
+                                      cl::cat(NMCat));
----------------
This is generally lowercase. 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D101835