[PATCH] D144529: [TextAPI] Add support for TBDv5 Files to nm & tapi-diff

[James Henderson via Phabricator via llvm-commits]

jhenderson added a comment.

I got pinged on this due to the llvm-nm change, so I took a quick look through and spotted a few nits/test issues. Someone who is more knowledgeable about TAPI/TBD etc stuff should properly review though.



================
Comment at: llvm/lib/TextAPI/Symbol.cpp:58-59
+bool Symbol::operator==(const Symbol &O) const {
+  // Older Tapi files do not express these all symbol flags, in those
+  // cases ignore those differences.
+  auto RemoveFlag = [](const Symbol &Sym, SymbolFlags &Flag) {
----------------



================
Comment at: llvm/test/Object/nm-tapi.test:71
+
+V5: /System/Library/Frameworks/Simple.framework/Versions/A/Simple (for architecture x86_64):
+V5-NEXT: 0000000000000000 D _OBJC_CLASS_$_Base
----------------
Aside: It was confusing that the llvm-nm related testing is not in the test/tools/llvm-nm folder, unlike most llvm-nm testing. Perhaps worth considering moving this test in a separate change?


================
Comment at: llvm/test/tools/llvm-tapi-diff/v5.test:3-4
+; RUN: split-file %s %t  
+; RUN: llvm-tapi-diff %t/Simple_v5.tbd  %t/Simple_v5.tbd | FileCheck %s --allow-empty
+; RUN: llvm-tapi-diff %t/Simple_v5.tbd  %t/Simple_v4.tbd | FileCheck %s --allow-empty
+
----------------
Presumably you need to add `2>&1` here to capture stderr for any warnings?


================
Comment at: llvm/test/tools/llvm-tapi-diff/v5.test:5
+; RUN: llvm-tapi-diff %t/Simple_v5.tbd  %t/Simple_v4.tbd | FileCheck %s --allow-empty
+
+
----------------
Nit: duplicate blank line.


================
Comment at: llvm/test/tools/llvm-tapi-diff/v5.test:7
+
+; CHECK-NOT: error:
+; CHECK-NOT: warning:
----------------
Isn't this unnecessary? Won't llvm-tapi-diff fail with a non-zero exit code if it encounters and error?


================
Comment at: llvm/test/tools/llvm-tapi-diff/v5.test:23
+{"main_library":{"exported_symbols":[{"targets":["x86_64-macos"],"text":{"global":["_foo"]}}],"flags":[{"attributes":["not_app_extension_safe"]}],"install_names":[{"name":"@rpath/libFake.dylib"}],"target_info":[{"min_deployment":"13","target":"x86_64-macos"},{"min_deployment":"13","target":"arm64-macos"}]},"tapi_tbd_version":5}
+
----------------
Nit: looks like you've got an extra trailing blank line.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D144529