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

[Jonas Devlieghere via Phabricator via llvm-commits]

JDevlieghere added inline comments.


================
Comment at: llvm/tools/llvm-tapi-diff/DiffEngine.cpp:9-10
+//
+// This header defines the implementation of the llvm-tapi difference
+// engine, which structurally compares two tbd files.
+//
----------------
This is not a header tho :-) 


================
Comment at: llvm/tools/llvm-tapi-diff/DiffEngine.cpp:92-95
+  ReturnedOutput.push_back(
+      getSingleAttrDiff(DiffScalarVal<StringRef, AD_Diff_Scalar_Str>(
+                            Order, Interface->getInstallName()),
+                        "Install Name"));
----------------
Could we reduce the density of this code by adding a helper that looks something like:

```
void DiffAttribute(StringRef Name, std::vector<DiffOutput>& Output, AttributeDiff Attr) {
  Output.push_back(getSingleAttrDiff(Attr, Name));
}
```

and then simplify these calls to:

```
DiffAttribute("Install Name", ReturnedOutput, DiffScalarVal<StringRef, AD_Diff_Scalar_Str>(Order, Interface->getInstallName()));
```

Having the name of the attribute first makes it clear what you're diffing. I'd probably also rename `ReturnedOutput` to something shorter (`Output`) to reduce the length of those calls. 


================
Comment at: llvm/tools/llvm-tapi-diff/DiffEngine.h:64-65
+  /// The order is the file from which the diff is found
+  InterfaceInputOrder Order;
+  T Val;
+  DiffScalarVal(InterfaceInputOrder Order, T Val)
----------------
Can these two be private? These members look like implementation details. Same in the other classes.


================
Comment at: llvm/tools/llvm-tapi-diff/DiffEngine.h:72
+  void print(raw_ostream &OS, std::string Indent) {
+    OS << Indent << "\t" << ((this->Order == lhs) ? "< " : "> ") << this->Val
+       << "\n";
----------------
I see `(this->Order == lhs) ? "< " : "> ")` in several places. How about creating a helper that takes a `InterfaceInputOrder` and returns a `StringRef` to avoid this duplication? 


================
Comment at: llvm/tools/llvm-tapi-diff/DiffEngine.h:132
+       << getFlagString(Val->getFlags()) << "\n";
+  }
+
----------------
Why are these functions implemented in the header?


================
Comment at: llvm/tools/llvm-tapi-diff/DiffEngine.h:171
+  MachO::Target Targ;
+  /// vector of StringRef values associated with the target
+  std::vector<DiffScalarVal<StringRef, AD_Diff_Scalar_Str>> DiffVec;
----------------
The LLVM Coding Guide says that comments should be sentences, i.e. capitalized and with a period at the end.


================
Comment at: llvm/tools/llvm-tapi-diff/DiffEngine.h:222
+  /// Function that prints the differences found in the files
+  void printDifferences(std::vector<DiffOutput>, int);
+  /// Function that does the comparison of the TBD files and returns the
----------------
You're passing the vector by value, so this will copy all its element for printing. Can this be a const ref?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D101835