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

[Cyndy Ishida via Phabricator via llvm-commits]

cishida added inline comments.


================
Comment at: llvm/tools/llvm-tapi-diff/DiffEngine.cpp:128
+      getSingleAttrDiff(Interface->symbols(), "Symbols", Order));
+  if (!Interface->documents().empty())
+    for (auto Doc : Interface->documents()) {
----------------
you don't need this conditional, the body of the for loop would never run if its empty


================
Comment at: llvm/tools/llvm-tapi-diff/DiffEngine.cpp:171
+void findAndAddDiff(
+    llvm::MachO::InterfaceFile::const_symbol_range CollectedSyms,
+    llvm::MachO::InterfaceFile::const_symbol_range LookupSyms,
----------------
These don't need to be scoped


================
Comment at: llvm/tools/llvm-tapi-diff/DiffEngine.h:56
+  /// from each file where a diff is present.
+  std::vector<std::unique_ptr<AttributeDiff>> DiffVec;
+  DiffOutput(std::string Name) : Name(Name){};
----------------
Most, if not all, instances of this type are named `Diff` Diff.DiffVec is redundant and is also used as a variable name in other places. 
What about `Values`?



================
Comment at: llvm/tools/llvm-tapi-diff/DiffEngine.h:81
+                                                    std::string Indent) {
+  OS << Indent << "\t\t" << ((this->Order == lhs) ? "< " : "> ") << this->Val
+     << "\n";
----------------
nit: `this->` calls are unecessarcy


================
Comment at: llvm/tools/llvm-tapi-diff/DiffEngine.h:101
+
+/// SymScalar is the diff symbol and the order. j
+class SymScalar {
----------------
Remove the typo at the end. 


================
Comment at: llvm/tools/llvm-tapi-diff/llvm-tapi-diff.cpp:1
+//===-- llvm-tapi-diff.cpp - symbol table comparator command-line driver ---*-
+// C++
----------------
cishida wrote:
> we should also replace `symbol table comparator` with something like `(tapi|tbd) file comparator`
sorry for the confusion on notation, I mean either tapi or tbd should be in the description.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D101835