[PATCH] D124792: [ifs] Add --strip-size flag

[James Henderson via Phabricator via llvm-commits]

jhenderson added a comment.

Three nits, but otherwise LGTM, with the caveat that I'm not a user or regular developer of this tool, so there might be some cases I haven't considered. If there's someone else available to sign off too, that would be good.



================
Comment at: llvm/docs/CommandGuide/llvm-ifs.rst:201-202
+ file. This is useful for shared objects that only intend to be linked against
+ position independent code which doesn't need copy relocations, where the size
+ of an object is not useful part of the abi to track.
+
----------------
Not sure if the "and" should be "or".


================
Comment at: llvm/lib/InterfaceStub/IFSHandler.cpp:121
     if (Symbol.Type == IFSSymbolType::NoType) {
-      IO.mapOptional("Size", Symbol.Size, (uint64_t)0);
-    } else if (Symbol.Type == IFSSymbolType::Func) {
-      Symbol.Size = 0;
-    } else {
-      IO.mapRequired("Size", Symbol.Size);
+      // Either Size is None, so we are reading it in, or it is non 0 then we
+      // should emit it.
----------------



================
Comment at: llvm/tools/llvm-ifs/llvm-ifs.cpp:440
+  if (StripSize)
+    for (auto &Sym : Stub.Symbols)
+      Sym.Size.reset();
----------------
I'd probably explicitly write out the type of `Sym`, since it is not obvious from this location (see the coding standards on use of `auto`).


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

https://reviews.llvm.org/D124792