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

[Jonas Devlieghere via Phabricator via llvm-commits]

JDevlieghere requested changes to this revision.
JDevlieghere added inline comments.
This revision now requires changes to proceed.


================
Comment at: llvm/include/llvm/Object/TapiUniversal.h:104
 
+  MachO::InterfaceFile *getInterfaceFile() { return ParsedFile.get(); }
+
----------------
It looks like the `TapiUniversal` guarantees that `ParsedFile` isn't `NULL` and indeed, some other functions seems to be unconditionally dereferencing this member. Should this return function a (const) reference instead? 


================
Comment at: llvm/include/llvm/TextAPI/Target.h:63
 
+std::string getTargetTriple(Target Targ);
+
----------------
cishida wrote:
> 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
You're passing `Target` by value, so this is going to make a copy. Is that intentional? Should this take a `const Target&` instead? 


================
Comment at: llvm/lib/TextAPI/Platform.cpp:117
+    return "driverkit" + Version;
+  }
+}
----------------
GCC used to complain about a missing return even after an exhaustive switch. Many places add an `llvm_unreachable` at the end of the function to avoid that. 


================
Comment at: llvm/test/tools/llvm-tapi-diff/tapi-diff-mismatched-number-of-inlines.test:1
+; RUN: NOT llvm-tapi-diff %S/Inputs/v4B.tbd %S/Inputs/v4D.tbd 2>&1 | FileCheck %s
+
----------------
The `not` binary is all lowercase. I suspect this will fail when running the test on a case-sensitive file system. 


================
Comment at: llvm/tools/llvm-tapi-diff/DiffEngine.cpp:24-26
+  if (std::distance(LHS.begin(), LHS.end()) !=
+      (std::distance(RHS.begin(), RHS.end())))
+    return false;
----------------
I was under the impression `std::equal` already ensured that the two ranges are of equal length. Is this meant as an optimization to quickly discard cases where the number of element are different? If so, keep in mind that for some iterator types like forward iterators, this is going to be O(n). Unless you know this is only going to be called with a random access iterator for which this operation is O(1) this might actually hurt performance. 


================
Comment at: llvm/tools/llvm-tapi-diff/DiffEngine.cpp:31
+
+template <typename T, typename U, typename V>
+void addValToDiffVec(V Val, Target Targ, DiffOutput &Diff,
----------------
`T`, `U` and `V` are pretty cryptic. It looks like `U` is some sort of `TargetVec` type, so maybe call it `TargetVecT`. 


================
Comment at: llvm/tools/llvm-tapi-diff/DiffEngine.h:23-42
+enum InterfaceInputOrder { lhs, rhs };
+
+enum DiffAttrKind {
+  AD_Diff_Scalar_PackedVersion,
+  AD_Diff_Scalar_Unsigned,
+  AD_Diff_Scalar_Bool,
+  AD_Diff_Scalar_Str,
----------------
The enums and classes in this header could use a Doxygen comment explaining what they are used for. To me it's not clear from their name what their purpose is. 


================
Comment at: llvm/tools/llvm-tapi-diff/DiffEngine.h:185
+public:
+  DiffEngine(object::TapiUniversal *InputFileNameOne,
+             object::TapiUniversal *InputFileNameTwo)
----------------
Above you use LHS and RHS. Is there a reason to use One and Two here? Personally I'd stick with the former.


================
Comment at: llvm/tools/llvm-tapi-diff/llvm-tapi-diff.cpp:32
+  if (EC) {
+    errs() << Path << ": " << EC.message() << "\n";
+    return true;
----------------
Please use `WithColor::error()` for consistency with other tools (and color output :-) 


================
Comment at: llvm/tools/llvm-tapi-diff/llvm-tapi-diff.cpp:42
+
+std::unique_ptr<Binary> convertFileToBinary(std::string &Filename) {
+  ErrorOr<std::unique_ptr<MemoryBuffer>> BufferOrErr =
----------------
Instead of dumping the error here, why not return it and let the caller deal with it? Below there's a call to `convertFileToBinary` and without checking the implementation it looks like it's lacking error handling. 


================
Comment at: llvm/tools/llvm-tapi-diff/llvm-tapi-diff.cpp:66
+    cl::PrintHelpMessage();
+    return 1;
+  }
----------------
Some/most llvm tools use `EXIT_SUCCESS` and `EXIT_FAILURE`, which make it much easier to quickly spot exit codes. 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D101835