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

[Jonas Devlieghere via Phabricator via llvm-commits]

JDevlieghere added inline comments.


================
Comment at: llvm/lib/TextAPI/Platform.cpp:134
+  }
+  llvm_unreachable("Unknown llvm.MachO.PlatformKind enum");
+}
----------------
Should this be `llvm::MachO::PlatformKind`? 


================
Comment at: llvm/tools/llvm-tapi-diff/DiffEngine.cpp:1
+//===-- DiffEngine.cpp - Structural file comparison ------===//
+//
----------------
This seems to be missing a few dashes. 


================
Comment at: llvm/tools/llvm-tapi-diff/DiffEngine.cpp:220-221
+
+std::vector<DiffOutput> DiffEngine::findDifferences(const InterfaceFile *IF1,
+                                                    const InterfaceFile *IF2) {
+  std::vector<DiffOutput> ReturnedOutput;
----------------
This is still using `1` and `2` while everything else is now using `LHS` and `RHS`. 


================
Comment at: llvm/tools/llvm-tapi-diff/DiffEngine.cpp:324-326
+    if (!Docs.DiffVec.empty()) {
+      ReturnedOutput.push_back(std::move(Docs));
+    }
----------------
The llvm style guidelines say no braces around single-line `if`s. 


================
Comment at: llvm/tools/llvm-tapi-diff/DiffEngine.h:48-58
+/// DiffOutput is the representation of a diff for a single attribute. The name
+/// is the name of the attribute. The kind is the enum value of DiffAttrKind for
+/// RTTI. DiffVec is the different scalar values (DiffScalarVal or SymScalar)
+/// for the attribute from each file where a diff is present.
+class DiffOutput {
+public:
+  std::string Name;
----------------
Rather than documenting the class as a whole, it's good practice to document the individual members like this. This is much more maintainable when a member gets added or removed, and Doxygen will associate the comment with the variable for you, which means you don't have to repeat the type etc. 


================
Comment at: llvm/tools/llvm-tapi-diff/DiffEngine.h:54-56
+  std::string Name;
+  DiffAttrKind Kind;
+  std::vector<std::unique_ptr<AttributeDiff>> DiffVec;
----------------
Should these members really be public? If the answer is yes, you'd want to use `struct` instead of `class`, which has public visibility by default. It's not so much about avoiding the access specifier on line 53, but more about conveying your intention to other developers.  


================
Comment at: llvm/tools/llvm-tapi-diff/DiffEngine.h:146
+      return "_OBJC_IVAR_$_";
+    }
+  }
----------------
This probably also needs an unreachable. Same for the function below. There might be more in this patch. 


================
Comment at: llvm/tools/llvm-tapi-diff/DiffEngine.h:228
+#endif
\ No newline at end of file

----------------
Several files seem to be missing newlines at the end. 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D101835