[PATCH] D133448: Make llvm-tli-checker support static libraries

[Renato Golin via Phabricator via llvm-commits]

rengolin added a comment.

A few nits, but looks good to me, thanks!



================
Comment at: llvm/tools/llvm-tli-checker/llvm-tli-checker.cpp:167
 
+// Insert defined global function symbols into the map.
+void SDKNameMap::maybeInsertSymbol(const SymbolRef &S, section_iterator End) {
----------------
Nit:

    // Insert defined global function symbols into the map if valid.


================
Comment at: llvm/tools/llvm-tli-checker/llvm-tli-checker.cpp:168
+// Insert defined global function symbols into the map.
+void SDKNameMap::maybeInsertSymbol(const SymbolRef &S, section_iterator End) {
+  SymbolRef::Type Type = unwrapIgnoreError(S.getType());
----------------
Feels weird to pass the end() iterator here, because in theory you could pass any valid iterator and it wouldn't be the same logic.

I think passing a reference to the object and then using `Section != o.section_end()` would be more clear as well as avoiding improper use by accident.


================
Comment at: llvm/tools/llvm-tli-checker/llvm-tli-checker.cpp:176
+      Section != End)
+    insert({ Name, true });
+}
----------------
Nit: I'd wrap this in curly brackets because of the two-line if statement.


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

https://reviews.llvm.org/D133448